答复: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

答复: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

[Yang Huajian（杨华健）]

Thank you for your reply!

In an earlier email I wrote:

> Some network devices that would not able to ping large packet under 
> bridge, but large packet ping is successful if not enable NF_CONNTRACK_BRIDGE.

If the ping test successed without NF_CONNTRACK_BRIDGE, it is because the netdev doesn't need such a large headroom in actual network forwarding.

If the netdev realy need it, the original bridge forwarding will fail too.

Maybe we need reconfig our wifi netdev or something else.

So is the nf_br_ip_fragment done to be consistent with the original bridge forwarding?

There are two very different ideas here:

One is to try to maintain the same treatment as the original bridge, as it is currently.

The other is to try to ensure that the packet is forwarded.

> I would prefer to keep blackhole logic for the mtu tests, i.e.
>  if (first_len - hlen > mtu)
>      goto blackhole;

Anyway, this modification is more appropriate.

Because I have tested by change mtu just now, goto slowpath cannot forward it either.


Best Regards,
Huajian

-----邮件原件-----
发件人: Florian Westphal [mailto:fw@strlen.de] 
发送时间: 2025年4月10日 18:18
收件人: Yang Huajian（杨华健） <huajianyang@asrmicro.com>
抄送: pablo@netfilter.org; fw@strlen.de; kadlec@netfilter.org; razor@blackwall.org; idosch@nvidia.com; davem@davemloft.net; dsahern@kernel.org; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; netfilter-devel@vger.kernel.org; coreteam@netfilter.org; bridge@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

Huajian Yang <huajianyang@asrmicro.com> wrote:
> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
> @@ -61,18 +61,14 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
>  		struct sk_buff *frag;
>  
>  		if (first_len - hlen > mtu ||
> -		    skb_headroom(skb) < ll_rs)
> -			goto blackhole;

I would prefer to keep blackhole logic for the mtu tests, i.e.
  if (first_len - hlen > mtu)
      goto blackhole;

same for the frag->len test in the skb_walk_frags loop.
From what I understood the problem is only because of the lower devices' headroom requirement.

[PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

[Huajian Yang]

The config NF_CONNTRACK_BRIDGE will change the bridge forwarding for
fragmented packets.

The original bridge does not know that it is a fragmented packet and
forwards it directly, after NF_CONNTRACK_BRIDGE is enabled, function
nf_br_ip_fragment and br_ip6_fragment will check the headroom.

In original br_forward, insufficient headroom of skb may indeed exist,
but there's still a way to save the skb in the device driver after
dev_queue_xmit.So droping the skb will change the original bridge
forwarding in some cases.

Signed-off-by: Huajian Yang <huajianyang@asrmicro.com>
---
 net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++------
 net/ipv6/netfilter.c                       | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 816bb0fde718..6482de4d8750 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -60,19 +60,19 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 		struct ip_fraglist_iter iter;
 		struct sk_buff *frag;
 
-		if (first_len - hlen > mtu ||
-		    skb_headroom(skb) < ll_rs)
+		if (first_len - hlen > mtu)
 			goto blackhole;
 
-		if (skb_cloned(skb))
+		if (skb_cloned(skb) ||
+		    skb_headroom(skb) < ll_rs)
 			goto slow_path;
 
 		skb_walk_frags(skb, frag) {
-			if (frag->len > mtu ||
-			    skb_headroom(frag) < hlen + ll_rs)
+			if (frag->len > mtu)
 				goto blackhole;
 
-			if (skb_shared(frag))
+			if (skb_shared(frag) ||
+			    skb_headroom(frag) < hlen + ll_rs)
 				goto slow_path;
 		}
 
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index 581ce055bf52..4541836ee3da 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -164,20 +164,20 @@ int br_ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 		struct ip6_fraglist_iter iter;
 		struct sk_buff *frag2;
 
-		if (first_len - hlen > mtu ||
-		    skb_headroom(skb) < (hroom + sizeof(struct frag_hdr)))
+		if (first_len - hlen > mtu)
 			goto blackhole;
 
-		if (skb_cloned(skb))
+		if (skb_cloned(skb) ||
+		    skb_headroom(skb) < (hroom + sizeof(struct frag_hdr)))
 			goto slow_path;
 
 		skb_walk_frags(skb, frag2) {
-			if (frag2->len > mtu ||
-			    skb_headroom(frag2) < (hlen + hroom + sizeof(struct frag_hdr)))
+			if (frag2->len > mtu)
 				goto blackhole;
 
 			/* Partially cloned skb? */
-			if (skb_shared(frag2))
+			if (skb_shared(frag2) ||
+			    skb_headroom(frag2) < (hlen + hroom + sizeof(struct frag_hdr)))
 				goto slow_path;
 		}
 
-- 
2.48.1

Re: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

[Florian Westphal]

Huajian Yang <huajianyang@asrmicro.com> wrote:
> The config NF_CONNTRACK_BRIDGE will change the bridge forwarding for
> fragmented packets.
> 
> The original bridge does not know that it is a fragmented packet and
> forwards it directly, after NF_CONNTRACK_BRIDGE is enabled, function
> nf_br_ip_fragment and br_ip6_fragment will check the headroom.
> 
> In original br_forward, insufficient headroom of skb may indeed exist,
> but there's still a way to save the skb in the device driver after
> dev_queue_xmit.So droping the skb will change the original bridge
> forwarding in some cases.

Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
Reviewed-by: Florian Westphal <fw@strlen.de>

This should probably be routed via Pablo.

Pablo, feel free to route this via nf-next if you think its not an
urgent fix, its been like this since bridge conntrack was added.

答复: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

[Yang Huajian（杨华健）]

Hi Pablo,

Can you give some advice?

Thanks.

Best Regards,
Huajian

-----邮件原件-----
发件人: Florian Westphal [mailto:fw@strlen.de] 
发送时间: 2025年4月17日 22:12
收件人: Yang Huajian（杨华健） <huajianyang@asrmicro.com>
抄送: pablo@netfilter.org; kadlec@netfilter.org; razor@blackwall.org; idosch@nvidia.com; davem@davemloft.net; dsahern@kernel.org; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; netfilter-devel@vger.kernel.org; coreteam@netfilter.org; bridge@lists.linux.dev; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

Huajian Yang <huajianyang@asrmicro.com> wrote:
> The config NF_CONNTRACK_BRIDGE will change the bridge forwarding for 
> fragmented packets.
> 
> The original bridge does not know that it is a fragmented packet and 
> forwards it directly, after NF_CONNTRACK_BRIDGE is enabled, function 
> nf_br_ip_fragment and br_ip6_fragment will check the headroom.
> 
> In original br_forward, insufficient headroom of skb may indeed exist, 
> but there's still a way to save the skb in the device driver after 
> dev_queue_xmit.So droping the skb will change the original bridge 
> forwarding in some cases.

Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
Reviewed-by: Florian Westphal <fw@strlen.de>

This should probably be routed via Pablo.

Pablo, feel free to route this via nf-next if you think its not an urgent fix, its been like this since bridge conntrack was added.

Re: [PATCH] net: Move specific fragmented packet to slow_path instead of dropping it

[Pablo Neira Ayuso]

On Thu, Apr 17, 2025 at 04:12:13PM +0200, Florian Westphal wrote:
> Huajian Yang <huajianyang@asrmicro.com> wrote:
> > The config NF_CONNTRACK_BRIDGE will change the bridge forwarding for
> > fragmented packets.
> > 
> > The original bridge does not know that it is a fragmented packet and
> > forwards it directly, after NF_CONNTRACK_BRIDGE is enabled, function
> > nf_br_ip_fragment and br_ip6_fragment will check the headroom.
> > 
> > In original br_forward, insufficient headroom of skb may indeed exist,
> > but there's still a way to save the skb in the device driver after
> > dev_queue_xmit.So droping the skb will change the original bridge
> > forwarding in some cases.
> 
> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
> Reviewed-by: Florian Westphal <fw@strlen.de>
> 
> This should probably be routed via Pablo.
> 
> Pablo, feel free to route this via nf-next if you think its not an
> urgent fix, its been like this since bridge conntrack was added.

Thanks, I will include this in the nf-next batch.