af-packet: respect vlan.use-for-tracking in eBPF bypass#15469
Conversation
AFPBypassCallback and AFPXDPBypassCallback populate the flow_table_v4 and flow_table_v6 eBPF maps using p->vlan_id[N] directly. When the matching xdp_filter.c program is built with VLAN_TRACKING=0 (i.e. vlan.use-for-tracking is false in suricata.yaml), the kernel side writes vlan0 and vlan1 as 0 on every map lookup, so userspace inserts that carry the real VLAN id never match and per-flow bypass counters stay at zero. Apply g_vlan_mask to each vlan_id assignment, matching the masking pattern already used by flow-hash.c. When vlan.use-for-tracking is true, g_vlan_mask is 0xffff and behaviour is unchanged. Original patch by John Graat on Redmine ticket 8242. Rebased to current main and dropped the now-removed vlan2 field. Ticket: 8242
for SV to run tests based on the presence of this feature
so as to run ebpf live tests
Ticket: 7674 Allows a compile-time option AFPACKET_TEST_REPLAY, that allows to set a configuration max-packets per afpacket interface, after which the PktAcqLoop stops. This allows suricata-verify tests to run with tcpreplay, and know when to stop
| #endif | ||
|
|
||
| #ifdef AFPACKET_TEST_REPLAY | ||
| uint32_t max_packets; |
There was a problem hiding this comment.
Please add a comment here explaining what it is, and the meaning of the special meaning of the 0 value
There was a problem hiding this comment.
This will be on me I guess
| StatsCounterIncr(&ptv->tv->stats, ptv->capture_afp_poll_data); | ||
| r = AFPReadFunc(ptv); | ||
| #ifdef AFPACKET_TEST_REPLAY | ||
| if (ptv->max_packets > 0 && ptv->pkts >= ptv->max_packets) { |
There was a problem hiding this comment.
would expect a simple explanation here as well
There was a problem hiding this comment.
also, this is comparing a uint64_t and uint32_t, should we cast max_packets to u64?
There was a problem hiding this comment.
should we cast max_packets to u64?
Only when we want to have SV test with more than U32_MAX packets
| - run: tar xf prep/suricata-verify.tar.gz | ||
| - run: ./autogen.sh | ||
| - run: CFLAGS="${DEFAULT_CFLAGS}" ./configure --enable-warnings --enable-unittests --enable-ebpf --enable-ebpf-build | ||
| - run: CFLAGS="${DEFAULT_CFLAGS} -DAFPACKET_TEST_REPLAY" ./configure --enable-warnings --enable-unittests --enable-ebpf --enable-ebpf-build |
There was a problem hiding this comment.
Generally we add configure options to define macros. Not sure how I feel about it here though. Opinions?
There was a problem hiding this comment.
Some context :
The goal here is to run SV tests with tcpreplay
So, we need a way for Suricata to know that it should stop
This is a test feature
So, I think it is ok this way, but if you want a configure option, I can do it this way
ptv->pkts is uint64_t and ptv->max_packets is uint32_t; cast the latter so the comparison is unsigned and same-width, addressing @victorjulien's review comment on OISF#15469.
Mirror the doc comment from the AFPThreadVars copy onto the config struct field, since they hold the same value (config -> per-thread copy at init).
Make sure these boxes are checked accordingly before submitting your Pull Request. Thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8242
Supersedes #15458 (opened as a fresh PR per @catenacyber's request).
SV_BRANCH=OISF/suricata-verify#3113
Describe changes
AFPBypassCallbackandAFPXDPBypassCallbackinsrc/source-af-packet.cnow ANDp->vlan_id[N]withg_vlan_maskbefore writing it into the eBPFflow_table_v4/flow_table_v6map keys, so the bypass key reflects the activevlan.use-for-trackingconfiguration instead of always including the raw VLAN id. Without this,vlan.use-for-tracking: nois silently ignored on the bypass path: tracked flows write keys with the VLAN id present, but the matching ingress lookup writes with the VLAN id zeroed, so the bypass never matches and the flow is reprocessed each packet.CI / SV coverage
This PR includes two commits from #15415, picked with @catenacyber's agreement, so SV coverage actually runs:
2e37d88(catenacyber)ci: add new xdp build with live tests: adds theubuntu-xdpjob to.github/workflows/builds.ymlthat runssuricata-verify/run.py --live --debug-failed(no equivalent job exists onmaintoday).762d2d7(catenacyber)test: new afpacket max-packets feature: adds theAFPACKET_TEST_REPLAYcompile-time feature (all changes#ifdef-guarded; only the newubuntu-xdpjob sets-DAFPACKET_TEST_REPLAY, so other builds are byte-identical), used by the SV live test in suricata-verify#3113 to stop Suricata cleanly after the replay.The matching SV PR suricata-verify#3113 that replays three VLAN-tagged UDP packets on the same 5-tuple through a dummy interface and asserts the bypass path engages with
vlan.use-for-tracking: no.