Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration: - [Low] The comment above nf_ct_bridge_frag_restore() (net/bridge/netfilter/nf_conntrack_bridge.c:396), "Actually only slow… -- netfilter: bridge: refragment packets with a vlan tag moved by the bridge With this patch, nf_ct_bridge_refrag() looks past a vlan tag that br_allowed_ingress() moved into the packet after prerouting defragmentation. It saves the tag, pulls it off, fragments the inner IPv4 or IPv6 packet, and then pushes the tag back onto each fragment in nf_ct_bridge_frag_restore() together with the outer protocol. Closes: https://syzkaller.appspot.com/bug?extid=a8ba738fe2db6b4bb27f > diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c > b/net/bridge/netfilter/nf_conntrack_bridge.c > index b5444335b86f..e16071eb2df0 100644 > --- a/net/bridge/netfilter/nf_conntrack_bridge.c > +++ b/net/bridge/netfilter/nf_conntrack_bridge.c [ ... ] > + proto = data.inband_vlan_encap_proto; > + __skb_pull(skb, VLAN_HLEN); > + skb->protocol = proto; [ ... ] > @@ -371,11 +399,18 @@ static int nf_ct_bridge_frag_restore(struct sk_buff > *skb, > { > int err; > > - err = skb_cow_head(skb, ETH_HLEN); > + err = skb_cow_head(skb, ETH_HLEN + (data->inband_vlan_present ? > VLAN_HLEN : 0)); > if (err) { > kfree_skb(skb); > return -ENOMEM; > } > + if (data->inband_vlan_present) { > + struct vlan_hdr *vhdr = skb_push(skb, VLAN_HLEN); > + > + vhdr->h_vlan_TCI = htons(data->inband_vlan_tci); > + vhdr->h_vlan_encapsulated_proto = data->inband_vlan_encap_proto; > + skb->protocol = data->inband_vlan_proto; > + } [Severity: Low] This isn't a bug, but the comment above nf_ct_bridge_frag_restore() hasn't been changed and still says: /* Actually only slow path refragmentation needs this. */ Now that nf_ct_bridge_refrag() pulls the vlan header and sets skb->protocol to the inner ethertype before it picks the fast or slow path, nf_ct_bridge_frag_restore() is the only code that pushes the header back. The frag_list fast paths also reach this function. nf_br_ip_fragment() and br_ip6_fragment() call output() for the head skb and for each fraglist member, and that leads to: output() nf_ct_bridge_refrag_post() nf_ct_bridge_frag_restore() Suppose a later change skipped the restore on the fast path, as the comment suggests is fine. The head fragment would then go out without its vlan header. data->mac would still hold the vlan TPID, and skb->protocol would be IP or IPv6. The comment was arguably wrong even before this patch, because fraglist members also need the MAC copy and the hwaccel tag. Since the fast path now depends on the vlan restore as well, could the comment be updated or removed? > if (data->vlan_present) > __vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci); > else if (skb_vlan_tag_present(skb)) -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020000308.37362-1-laveeshb%40laveeshbansal.com
