| Message ID | 20260928161830.351199-1-parri.andrea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [nf] netfilter: br_netfilter: restore VLAN tag on refragmented IPv6 packets | expand |
On Mon, Sep 28, 2026 at 04:19:47PM +0000, netdev-bot+sinfo@kernel.org wrote: > This is an automated message. This series looks like a fix, but its > commit messages seem to be missing some information: > > - Whether the issue was actually triggered, or is only theoretical > (e.g. found by code inspection). If it was triggered please include > the symptoms, like the stack trace or error messages. The issue was triggered, not only found by code inspection. I reproduced it under virt-ng with a local test on a VLAN-aware bridge with br_netfilter and IPv6 conntrack defrag enabled. The test sends a fragmented IPv4 packet on one VLAN, then a fragmented IPv6 packet on another VLAN that the bridge floods to several ports. The test is available on request. Symptoms: with the patch not applied, the refragmented IPv6 packets leave the bridge with the wrong VLAN tag. This is the tag of the earlier IPv4 flow when both refragmentations run on the same CPU. Otherwise the packets go out untagged. There is no crash or warning; the only effect is the wrong or missing tag on the wire. With the patch applied, all the IPv6 fragments keep the correct VLAN tag. Thanks, Andrea
[ Resending with the full Cc list, which I dropped by mistake in my previous reply. ] On Mon, Sep 28, 2026 at 04:19:47PM +0000, netdev-bot+sinfo@kernel.org wrote: > This is an automated message. This series looks like a fix, but its > commit messages seem to be missing some information: > > - Whether the issue was actually triggered, or is only theoretical > (e.g. found by code inspection). If it was triggered please include > the symptoms, like the stack trace or error messages. The issue was triggered, not only found by code inspection. I reproduced it under virt-ng with a local test on a VLAN-aware bridge with br_netfilter and IPv6 conntrack defrag enabled. The test sends a fragmented IPv4 packet on one VLAN, then a fragmented IPv6 packet on another VLAN that the bridge floods to several ports. The test is available on request. Symptoms: with the patch not applied, the refragmented IPv6 packets leave the bridge with the wrong VLAN tag. This is the tag of the earlier IPv4 flow when both refragmentations run on the same CPU. Otherwise the packets go out untagged. There is no crash or warning; the only effect is the wrong or missing tag on the wire. With the patch applied, all the IPv6 fragments keep the correct VLAN tag. Thanks, Andrea
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 1 · Medium: 0 · Low: 0 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). - [High] The fix is incomplete. -- netfilter: br_netfilter: restore VLAN tag on refragmented IPv6 packets This commit moves saving the L2 header and VLAN tag into a new helper, br_nf_save_frag_data(). Both the IPv4 and IPv6 refragmentation branches of br_nf_dev_queue_xmit() now call it. Before this change, the IPv6 branch reused whatever VLAN tag the last IPv4 refragmentation on that CPU had left in per-CPU storage. > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c > index 0a394e5f4391..ec69d6254e34 100644 > --- a/net/bridge/br_netfilter_hooks.c > +++ b/net/bridge/br_netfilter_hooks.c > @@ -832,6 +832,25 @@ static unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb) > return 0; > } > > +/* Fragments may not inherit the MAC header or VLAN tag. */ > +static void br_nf_save_frag_data(struct sk_buff *skb) > +{ > + struct brnf_frag_data *data = this_cpu_ptr(&brnf_frag_data_storage); > + > + if (skb_vlan_tag_present(skb)) { > + data->vlan_tci = skb->vlan_tci; > + data->vlan_proto = skb->vlan_proto; > + } else { > + data->vlan_proto = 0; > + } [Severity: High] What happens when the head skb goes out untagged but the fragments still carry a tag? For an untagged packet this helper saves vlan_proto = 0. The consumer only ever sets a tag and never clears one: net/bridge/br_netfilter_hooks.c:br_nf_push_frag_xmit() { ... if (data->vlan_proto) __vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci); ... } For IPv6, nf_ct_frag6_reasm() calls inet_frag_reasm_finish(..., false). Each original fragment stays as its own skb on the head's frag_list and keeps its own ingress vlan_all. On egress to an untagged VLAN (or a BR_VLAN_TUNNEL port), br_handle_vlan() clears the tag on the head skb only: if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED && !br_switchdev_frame_uses_tx_fwd_offload(skb)) __vlan_hwaccel_clear_tag(skb); A unicast-forwarded reassembled skb normally takes the skb_has_frag_list() fast path in ip6_fragment(). On that path, ip6_fraglist_prepare()->ip6_copy_metadata() does not copy or reset vlan_all. So would fragments 2..N reach br_nf_push_frag_xmit()->br_dev_queue_push_xmit() with their ingress 802.1Q/802.1ad tag still set, while the first fragment goes out untagged? The conntrack bridge restore path handles this case: net/bridge/netfilter/nf_conntrack_bridge.c:nf_ct_bridge_frag_restore() { ... if (data->vlan_present) __vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci); else if (skb_vlan_tag_present(skb)) __vlan_hwaccel_clear_tag(skb); ... } Without a similar else branch in br_nf_push_frag_xmit(), does the commit message's claim that IPv6 fragments now "use the current packet's VLAN information" still hold? The same branch would also cover the IPv4 path, since ip_copy_metadata() does not touch vlan_all either. Also, the comment "Fragments may not inherit the MAC header or VLAN tag" does not seem accurate for reused frag_list skbs, which keep their own tag. [ ... ]
On Thu, Oct 01, 2026 at 10:19:55AM +0000, netdev-bot+sashiko@kernel.org wrote: > For an untagged packet this helper saves vlan_proto = 0. The consumer only > ever sets a tag and never clears one: [...] > So would fragments 2..N reach br_nf_push_frag_xmit()->br_dev_queue_push_xmit() > with their ingress 802.1Q/802.1ad tag still set, while the first fragment > goes out untagged? Yes. Conntrack defrag keeps the original fragments on frag_list for both IPv6 and IPv4 here (ip_frag_coalesce_ok() is true only for local delivery), each with its ingress tag, and br_handle_vlan() clears the tag of the head skb only. On the fast path those skbs are sent as the fragments. I extended my reproducer with a port that is untagged in VLAN 10 and a static FDB entry, so the packet is not cloned. With this patch applied, 6/9 IPv6 and 6/9 IPv4 fragments left that port with "vlan 10"; only the first fragment of each packet was untagged. This is not introduced by this patch, but it means the changelog claim was too broad. v2 will add a second patch that clears the tag in br_nf_push_frag_xmit() when none was saved, as nf_ct_bridge_frag_restore() does, and narrow the changelog of this one to the fragments ip6_fragment() builds. The BR_VLAN_TUNNEL case does not reach the refragmentation code: br_handle_egress_vlan_tunnel() attaches a metadata dst, so br_nf_dev_queue_xmit() drops the packet at the !skb_valid_dst() check. > Also, the comment "Fragments may not inherit the MAC header or VLAN tag" > does not seem accurate for reused frag_list skbs, which keep their own tag. Agreed, v2 rewords it. pw-bot: cr Thanks, Andrea
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c index 0a394e5f43916..ec69d6254e340 100644 --- a/net/bridge/br_netfilter_hooks.c +++ b/net/bridge/br_netfilter_hooks.c @@ -832,6 +832,25 @@ static unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb) return 0; } +/* Fragments may not inherit the MAC header or VLAN tag. */ +static void br_nf_save_frag_data(struct sk_buff *skb) +{ + struct brnf_frag_data *data = this_cpu_ptr(&brnf_frag_data_storage); + + if (skb_vlan_tag_present(skb)) { + data->vlan_tci = skb->vlan_tci; + data->vlan_proto = skb->vlan_proto; + } else { + data->vlan_proto = 0; + } + + data->encap_size = nf_bridge_encap_header_len(skb); + data->size = ETH_HLEN + data->encap_size; + + skb_copy_from_linear_data_offset(skb, -data->size, data->mac, + data->size); +} + static int br_nf_dev_queue_xmit(struct net *net, struct sock *sk, struct sk_buff *skb) { struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb); @@ -866,28 +885,13 @@ static int br_nf_dev_queue_xmit(struct net *net, struct sock *sk, struct sk_buff */ if (IS_ENABLED(CONFIG_NF_DEFRAG_IPV4) && skb->protocol == htons(ETH_P_IP)) { - struct brnf_frag_data *data; - if (br_validate_ipv4(net, skb)) goto drop; IPCB(skb)->frag_max_size = nf_bridge->frag_max_size; local_lock_nested_bh(&brnf_frag_data_storage.bh_lock); - data = this_cpu_ptr(&brnf_frag_data_storage); - - if (skb_vlan_tag_present(skb)) { - data->vlan_tci = skb->vlan_tci; - data->vlan_proto = skb->vlan_proto; - } else { - data->vlan_proto = 0; - } - - data->encap_size = nf_bridge_encap_header_len(skb); - data->size = ETH_HLEN + data->encap_size; - - skb_copy_from_linear_data_offset(skb, -data->size, data->mac, - data->size); + br_nf_save_frag_data(skb); ret = br_nf_ip_fragment(net, sk, skb, br_nf_push_frag_xmit); local_unlock_nested_bh(&brnf_frag_data_storage.bh_lock); @@ -895,20 +899,13 @@ static int br_nf_dev_queue_xmit(struct net *net, struct sock *sk, struct sk_buff } if (IS_ENABLED(CONFIG_NF_DEFRAG_IPV6) && skb->protocol == htons(ETH_P_IPV6)) { - struct brnf_frag_data *data; - if (br_validate_ipv6(net, skb)) goto drop; IP6CB(skb)->frag_max_size = nf_bridge->frag_max_size; local_lock_nested_bh(&brnf_frag_data_storage.bh_lock); - data = this_cpu_ptr(&brnf_frag_data_storage); - data->encap_size = nf_bridge_encap_header_len(skb); - data->size = ETH_HLEN + data->encap_size; - - skb_copy_from_linear_data_offset(skb, -data->size, data->mac, - data->size); + br_nf_save_frag_data(skb); ret = ip6_fragment(net, sk, skb, br_nf_push_frag_xmit); local_unlock_nested_bh(&brnf_frag_data_storage.bh_lock);
Bridged IPv6 packets can lose their VLAN tag or acquire an unrelated tag when conntrack-reassembled packets are refragmented. On a VLAN-aware bridge, this can send fragments with a different VLAN tag from the one selected for forwarding. br_nf_push_frag_xmit() restores the VLAN tag from per-CPU storage, but only the IPv4 branch of br_nf_dev_queue_xmit() saves it. The IPv6 branch leaves the saved tag from the previous IPv4 refragmentation on that CPU. Newly allocated fragments do not inherit the tag through ip6_copy_metadata(). Commit d7b597421519 ("netfilter: bridge: restore vlan tag when refragmenting") added VLAN tag restoration for IPv4 only, shortly after IPv6 refragmentation was introduced. Move saving the L2 header and VLAN tag into br_nf_save_frag_data() and call it from both branches, so IPv6 fragments use the current packet's VLAN information. Fixes: efb6de9b4ba0 ("netfilter: bridge: forward IPv6 fragmented packets") Cc: stable@vger.kernel.org Assisted-by: LLM Signed-off-by: Andrea Parri <parri.andrea@gmail.com> --- net/bridge/br_netfilter_hooks.c | 45 +++++++++++++++------------------ 1 file changed, 21 insertions(+), 24 deletions(-)