Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] In bridge_ct_vlan_proto.sh, the check "FAIL: conntrack did not
reassemble the packets" cannot detect missing reassembly in the vlan…
- [Low] In bridge_ct_vlan_proto.sh, run_all() returns ksft_skip as soon
as any run_case() returns ksft_skip, even after earlier cases have run.
--
selftests: netfilter: add bridge conntrack test for vlan protocol mismatch
This adds a selftest that sends fragmented IPv4 and IPv6 pings through a
vlan filtering bridge with bridge conntrack, using vlan tags whose
protocol differs from (or matches) the bridge's vlan_protocol. It checks
that the pings get through, that conntrack reassembled the packets, and
that the kernel did not warn.
> diff --git a/tools/testing/selftests/net/netfilter/bridge_ct_vlan_proto.sh
> b/tools/testing/selftests/net/netfilter/bridge_ct_vlan_proto.sh
> new file mode 100755
> index 000000000000..19f842e17060
> --- /dev/null
> +++ b/tools/testing/selftests/net/netfilter/bridge_ct_vlan_proto.sh
> @@ -0,0 +1,182 @@
[ ... ]
> + # Loading a ct rule in the bridge family enables bridge conntrack. The
> + # postrouting counter sees the packets reassembled by conntrack.
> + ip netns exec "$ns0" nft -f - <<EOF || return "$ksft_skip"
> +table bridge t {
> + chain pre {
> + type filter hook prerouting priority 0; policy accept;
> + ct state new counter
> + }
> + chain post {
> + type filter hook postrouting priority 0; policy accept;
> + meta length > 1400 counter
> + }
> +}
> +EOF
[Severity: Medium]
Can this counter go up in the mismatch cases even when nothing was
reassembled?
In the 802.1ad/802.1q, 802.1q/802.1ad and clone cases, __allowed_ingress()
moves the tag into the packet after prerouting:
net/bridge/br_vlan.c:__allowed_ingress() {
...
if (skb->vlan_proto != br->vlan_proto) {
/* Protocol-mismatch, empty out vlan_tci for new tag */
skb_push(skb, ETH_HLEN);
skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
skb_vlan_tag_get(skb));
...
}
After that, skb->data sits at the 4 byte in-band vlan header. meta length
is skb->len, so the post chain sees the IP length plus 4.
With the 1400 byte mtu on eth0.100, each IPv6 fragment is
40 + 8 + 1352 = 1400 bytes. A fragment that was never reassembled shows up
in the post chain as 1404 bytes and matches "meta length > 1400". IPv4
fragments come out at exactly 1400 and don't match.
If bridge conntrack stopped defragmenting, nf_ct_bridge_refrag() would
return early:
net/bridge/netfilter/nf_conntrack_bridge.c:nf_ct_bridge_refrag() {
...
if (!BR_INPUT_SKB_CB(skb)->frag_max_size)
return NF_ACCEPT;
...
}
The fragments would still be forwarded, both pings would succeed, and the
"packets [1-9]" check in run_case() would still pass. The refragmentation
path this test targets would then go untested and nothing would flag it.
Also, run_case() checks this one shared counter only once, after both the
IPv4 and IPv6 pings. So the check can't tell which family was reassembled.
For the mismatch cases, the comment "The postrouting counter sees the
packets reassembled by conntrack" doesn't look accurate either.
Would one of these work instead: put the counter in prerouting, where the
tag is still hwaccel; use a threshold above any fragment size (for example
> 1500); or keep separate "meta nfproto ipv4" and "meta nfproto ipv6"
counters and check each one after its own ping?
[ ... ]
> +run_all()
> +{
> + local rc
> +
> + # bridge protocol, protocol of the vlan devices
> + for case in "802.1q 802.1q" "802.1ad 802.1q" "802.1q 802.1ad" "802.1ad
> 802.1q clone"; do
> + # shellcheck disable=SC2086
> + run_case $case
> + rc=$?
> + if [ "$rc" -eq "$ksft_skip" ]; then
> + return "$ksft_skip"
> + fi
> + [ "$rc" -ne 0 ] && ret=1
> + done
> +
> + return 0
> +}
> +
> +run_all
> +rc=$?
> +if [ "$rc" -eq "$ksft_skip" ] && [ "$ret" -eq 0 ]; then
> + exit "$ksft_skip"
> +fi
> +
> +read -r t < /proc/sys/kernel/tainted
> +if [ $((t & taint_warn)) -ne 0 ]; then
> + echo "FAIL: kernel warned"
> + ret=1
> +fi
[Severity: Low]
Is it possible for a warning from an earlier case to get lost here?
run_all() returns ksft_skip as soon as any run_case() returns ksft_skip,
even when earlier cases have already run. The top level then exits with
ksft_skip before it reads /proc/sys/kernel/tainted.
Say an earlier case triggered a WARN but its pings still passed. ping -c 3
exits 0 if any reply arrives, so ret stays 0. If setup then fails in a
later case, for example because the 802.1ad bridge or vlan device can't
be created, the test reports SKIP and the WARN never gets reported.
The commit message says the test checks "that the kernel did not warn".
Could the taint check run before the skip decision, or could the test
fail whenever TAINT_WARN appears during the run?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020000308.37362-1-laveeshb%40laveeshbansal.com
