Bug fix #6238 and AF_XDP program loading #9315

joser12345678 · 2023-08-01T12:58:22Z

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.

[JR ] I have read the contributing guide lines at https://docs.suricata.io/en/latest/devguide/codebase/contributing/contribution-process.html
[JR ] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
[ JR] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

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 where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

fixed bug by adding program loading support and program unloading after suricata stops

github-actions · 2023-08-01T18:11:03Z

NOTE: This PR may contain new authors:

joser12345678 <joser17@vt.edu>

fixed bug by adding program loading support and program unloading after suricata stops

jufajardini · 2023-08-01T19:12:45Z

Hi there @joser12345678, did you close this on purpose?

joser12345678 · 2023-08-01T19:21:46Z

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

jufajardini · 2023-08-01T19:44:13Z

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 ^^
Your assumption was partially correct: when there are failures introduced by the PR, we don't usually close the PR right away, but we do ask the author to submit a new one, fixing said failures. I asked because I was in the process of reviewing it. Since you'll open a new PR soon, I'll leave a few general comments regarding our contribution guidelines here, so you can incorporate those on your next one! :)

jufajardini · 2023-08-01T20:19:58Z

Commits segmentation : if the new additions are necessary to fix the bug, then I think it's ok
Commit messages : please see separate comment
Git ID set : please use First and Last name for commit author
CLA : to confirm
code: please see inline comments. I don't have the expertise to review these changes in full, and to be honest I'm not sure I understand the need to load another program to fix the bug - nor how safe this option would be. If this isn't a requirement, I think it might be best to keep the two changes separate, if possible. As for the security aspect, I think that @catenacyber or Victor would be better judges, when we have a new version of the PR :)
Doc update : will probably be necessary, as this would introduce a new config option
Redmine ticket : ok
Rustfmt : to see with next PR
Tests : is it possible to create some suricata-verify test for this? that would be ideal
Dependencies added: don't think so

jufajardini

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!

jufajardini · 2023-08-01T19:54:05Z

src/source-af-xdp.c

+ * ADDED function to add a socket to the XDP map
+ */


Is this ADDED a left over that should be replaced by \brief?

jufajardini · 2023-08-01T19:59:46Z

src/runmode-af-xdp.c

+        SCLogInfo("unable to find af-xdp config for "
+                  "interface \"%s\" or \"default\", using default values",
+                live_dev);


Since this is related to configuration, it would probably be better to use SCLogConfig here and in other similar output messages, instead.

jufajardini · 2023-08-01T20:11:51Z

src/runmode-af-xdp.c

+    // 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) {


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?

jufajardini · 2023-08-01T20:13:32Z

src/runmode-af-xdp.c

+
+    int ret = xdp_program__detach(prog, ifindex, XDP_MODE_NATIVE, 0);
+    if (ret) {
+        printf("ERROR: can't detatch\n");


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?

Or, even if this is an unrecoverable error, some other option would be best.

jufajardini · 2023-08-01T20:15:08Z

src/source-af-xdp.c

+
+    struct xdp_multiprog *progs = xdp_multiprog__get_from_ifindex(ptv->ifindex);
+    if (progs == NULL) {
+        printf("cannot get multiprog");


The same comment about log API applies here :)

joser12345678 · 2023-08-02T12:33:47Z

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:
https://bugzilla.kernel.org/show_bug.cgi?id=217712

Bug fix OISF#6238 and AF_XDP program loading

f0d3157

fixed bug by adding program loading support and program unloading after suricata stops

joser12345678 requested a review from victorjulien as a code owner August 1, 2023 12:58

Bug fix OISF#6238 and AF_XDP program loading

557498d

fixed bug by adding program loading support and program unloading after suricata stops

joser12345678 closed this Aug 1, 2023

jufajardini reviewed Aug 1, 2023

View reviewed changes

jufajardini mentioned this pull request Aug 2, 2023

Bug fix #6238 and AF_XDP program loading #9319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix #6238 and AF_XDP program loading #9315

Bug fix #6238 and AF_XDP program loading #9315

joser12345678 commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

jufajardini commented Aug 1, 2023

joser12345678 commented Aug 1, 2023

jufajardini commented Aug 1, 2023 •

edited

Loading

jufajardini commented Aug 1, 2023

jufajardini left a comment

jufajardini Aug 1, 2023

jufajardini Aug 1, 2023

jufajardini Aug 1, 2023

jufajardini Aug 1, 2023

jufajardini Aug 1, 2023

jufajardini Aug 1, 2023

joser12345678 commented Aug 2, 2023

Bug fix #6238 and AF_XDP program loading #9315

Bug fix #6238 and AF_XDP program loading #9315

Conversation

joser12345678 commented Aug 1, 2023

Provide values to any of the below to override the defaults.

github-actions bot commented Aug 1, 2023

jufajardini commented Aug 1, 2023

joser12345678 commented Aug 1, 2023

jufajardini commented Aug 1, 2023 • edited Loading

jufajardini commented Aug 1, 2023

jufajardini left a comment

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

jufajardini Aug 1, 2023

Choose a reason for hiding this comment

joser12345678 commented Aug 2, 2023

jufajardini commented Aug 1, 2023 •

edited

Loading