-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fix #6238 and AF_XDP program loading #9315
Conversation
fixed bug by adding program loading support and program unloading after suricata stops
NOTE: This PR may contain new authors:
|
fixed bug by adding program loading support and program unloading after suricata stops
Hi there @joser12345678, did you close this on purpose? |
Yes sir, I was under the impression that since my commit commit failed the tests here it was going to be closed and I would have to open a new one anyway. I take it this was an incorrect assumption on my part. I apologize as this is my first time contributing to suricata let alone an open source project |
No problems! Welcome to our community, and thanks for your interest in contributing to Suricata! I hope you'll have a good open source experience with us ^^ |
Commits segmentation : if the new additions are necessary to fix the bug, then I think it's ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few comments for you to consider for your next PR :)
On top of those, for your next commit message, please follow our guideline of:
module: brief describe changes (<50 chars)
Add more descriptive message.
Bug #6238
If anything from my comments isn't clear, please, ask away!
* ADDED function to add a socket to the XDP map | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ADDED
a left over that should be replaced by \brief
?
SCLogInfo("unable to find af-xdp config for " | ||
"interface \"%s\" or \"default\", using default values", | ||
live_dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is related to configuration, it would probably be better to use SCLogConfig
here and in other similar output messages, instead.
// get the path to the xdp-program | ||
const char *program_path = NULL; | ||
const char *confstr = NULL; | ||
if (ConfGetChildValueWithDefault(if_root, if_default, "xdp-program", &confstr) == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm missing something. Shouldn't xdp-program
be added to our suricata.yaml.in
if you want to use that config value?
|
||
int ret = xdp_program__detach(prog, ifindex, XDP_MODE_NATIVE, 0); | ||
if (ret) { | ||
printf("ERROR: can't detatch\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use our own SCLog API for logging messages with adequate log levels. If these are debug messages, I think SCLogDebug
would work better, if not, maybe SCLogNotice
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, even if this is an unrecoverable error, some other option would be best.
|
||
struct xdp_multiprog *progs = xdp_multiprog__get_from_ifindex(ptv->ifindex); | ||
if (progs == NULL) { | ||
printf("cannot get multiprog"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment about log API applies here :)
Ok, for my next version of this commit I will separate the adding of an AF_XDP program. The bug is detailed in the bug ticket. If more info is needed: |
fixed bug by adding program loading support and program unloading after suricata stops. This is done by adding support for adding and removing AF_XDP programs.
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6238
Describe changes:
fixed bug by adding program loading support and program unloading after suricata stops. This is done by adding support for adding and removing AF_XDP programs. Added RunModeAFXDPRemoveProg() and RunModeLoadAFXDPProg() in runmode-af-xdp.c. Also added flag in the AFXDPIfaceConfig struct called inhibit_prog_load which if true will set a libxdp flag that prevents the default AF_XDP program from being loaded; this is needed if the user wants to load their own AF_XDP program. source-af-xdp.c was also changed to act on this flag being set. The user can now add the "xdp-program" item in the suricata.yaml file to point to a xdp-program the user wishes to load. Finally, added check in suricata.c to unload xdp-programs before the threads are shutdown, which fixes the race condition described in the ticket.
Provide values to any of the below to override the defaults.
To use a pull request use a branch name like
pr/N
whereN
is thepull request number.
Alternatively,
SV_BRANCH
may also be a link to anOISF/suricata-verify pull-request.