linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error
@ 2021-07-26  3:57 Yajun Deng
  2021-07-28 16:18 ` Pablo Neira Ayuso
  2021-07-29  3:19 ` yajun.deng
  0 siblings, 2 replies; 4+ messages in thread
From: Yajun Deng @ 2021-07-26  3:57 UTC (permalink / raw)
  To: pablo, kadlec, fw, roopa, nikolay, davem, kuba
  Cc: netfilter-devel, coreteam, bridge, netdev, linux-kernel, Yajun Deng

It should be added kfree_skb_list() when err is not equal to zero
in nf_br_ip_fragment().

Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 8d033a75a766..059f53903eda 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -83,12 +83,16 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 
 			skb->tstamp = tstamp;
 			err = output(net, sk, data, skb);
-			if (err || !iter.frag)
-				break;
-
+			if (err) {
+				kfree_skb_list(iter.frag);
+				return err;
+			}
+
+			if (!iter.frag)
+				return 0;
+
 			skb = ip_fraglist_next(&iter);
 		}
-		return err;
 	}
 slow_path:
 	/* This is a linearized skbuff, the original geometry is lost for us.
-- 
2.32.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error
  2021-07-26  3:57 [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error Yajun Deng
@ 2021-07-28 16:18 ` Pablo Neira Ayuso
  2021-07-29  3:19 ` yajun.deng
  1 sibling, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2021-07-28 16:18 UTC (permalink / raw)
  To: Yajun Deng
  Cc: kadlec, fw, roopa, nikolay, davem, kuba, netfilter-devel,
	coreteam, bridge, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On Mon, Jul 26, 2021 at 11:57:02AM +0800, Yajun Deng wrote:
> It should be added kfree_skb_list() when err is not equal to zero
> in nf_br_ip_fragment().
> 
> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
>  net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
> index 8d033a75a766..059f53903eda 100644
> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
> @@ -83,12 +83,16 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
>  
>  			skb->tstamp = tstamp;
>  			err = output(net, sk, data, skb);
> -			if (err || !iter.frag)
> -				break;
> -
> +			if (err) {
> +				kfree_skb_list(iter.frag);
> +				return err;
> +			}
> +
> +			if (!iter.frag)
> +				return 0;
> +
>  			skb = ip_fraglist_next(&iter);
>  		}
> -		return err;

Why removing this line above? It enters slow_path: on success.

This patch instead will keep this aligned with IPv6.

>  	}
>  slow_path:
>  	/* This is a linearized skbuff, the original geometry is lost for us.
> -- 
> 2.32.0
> 

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 460 bytes --]

diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index 8d033a75a766..3cf5457919c6 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -88,6 +88,11 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
 
 			skb = ip_fraglist_next(&iter);
 		}
+
+		if (!err)
+			return 0;
+
+		kfree_skb_list(iter.frag_list);
 		return err;
 	}
 slow_path:

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error
  2021-07-26  3:57 [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error Yajun Deng
  2021-07-28 16:18 ` Pablo Neira Ayuso
@ 2021-07-29  3:19 ` yajun.deng
  2021-07-29  7:24   ` Pablo Neira Ayuso
  1 sibling, 1 reply; 4+ messages in thread
From: yajun.deng @ 2021-07-29  3:19 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: kadlec, fw, roopa, nikolay, davem, kuba, netfilter-devel,
	coreteam, bridge, netdev, linux-kernel

July 29, 2021 12:18 AM, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:

> On Mon, Jul 26, 2021 at 11:57:02AM +0800, Yajun Deng wrote:
> 
>> It should be added kfree_skb_list() when err is not equal to zero
>> in nf_br_ip_fragment().
>> 
>> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>> ---
>> net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c
>> b/net/bridge/netfilter/nf_conntrack_bridge.c
>> index 8d033a75a766..059f53903eda 100644
>> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
>> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
>> @@ -83,12 +83,16 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
>> 
>> skb->tstamp = tstamp;
>> err = output(net, sk, data, skb);
>> - if (err || !iter.frag)
>> - break;
>> -
>> + if (err) {
>> + kfree_skb_list(iter.frag);
>> + return err;
>> + }
>> +
>> + if (!iter.frag)
>> + return 0;
>> +
>> skb = ip_fraglist_next(&iter);
>> }
>> - return err;
> 
> Why removing this line above? It enters slow_path: on success.
> 
I used return rather than break, it wouldn't enter the slow_path.
> This patch instead will keep this aligned with IPv6.
> 
I think err and !iter.frag are not related, there is no need to put them in an if statement,
We still need to separate them after loop. So I separate them in loop and use return instead
of break. In addition, if you insist, I will accept your patch.
>> }
>> slow_path:
>> /* This is a linearized skbuff, the original geometry is lost for us.
>> --
>> 2.32.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error
  2021-07-29  3:19 ` yajun.deng
@ 2021-07-29  7:24   ` Pablo Neira Ayuso
  0 siblings, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2021-07-29  7:24 UTC (permalink / raw)
  To: yajun.deng
  Cc: kadlec, fw, roopa, nikolay, davem, kuba, netfilter-devel,
	coreteam, bridge, netdev, linux-kernel

On Thu, Jul 29, 2021 at 03:19:01AM +0000, yajun.deng@linux.dev wrote:
> July 29, 2021 12:18 AM, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> 
> > On Mon, Jul 26, 2021 at 11:57:02AM +0800, Yajun Deng wrote:
> > 
> >> It should be added kfree_skb_list() when err is not equal to zero
> >> in nf_br_ip_fragment().
> >> 
> >> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
> >> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> >> ---
> >> net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++++----
> >> 1 file changed, 8 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c
> >> b/net/bridge/netfilter/nf_conntrack_bridge.c
> >> index 8d033a75a766..059f53903eda 100644
> >> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
> >> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
> >> @@ -83,12 +83,16 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
> >> 
> >> skb->tstamp = tstamp;
> >> err = output(net, sk, data, skb);
> >> - if (err || !iter.frag)
> >> - break;
> >> -
> >> + if (err) {
> >> + kfree_skb_list(iter.frag);
> >> + return err;
> >> + }
> >> +
> >> + if (!iter.frag)
> >> + return 0;
> >> +
> >> skb = ip_fraglist_next(&iter);
> >> }
> >> - return err;
> > 
> > Why removing this line above? It enters slow_path: on success.
> > 
> I used return rather than break, it wouldn't enter the slow_path.

Right, your patch is correct.

> > This patch instead will keep this aligned with IPv6.
> > 
> I think err and !iter.frag are not related, there is no need to put
> them in an if statement, We still need to separate them after loop.
> So I separate them in loop and use return instead of break. In
> addition, if you insist, I will accept your patch.

Thanks. Yes, I'd prefer to keep it consistent with existing users of
the fragment iterator, see:

net/ipv4/ip_output.c
net/ipv6/netfilter.c
net/ipv6/ip6_output.c

they are roughly using the same programming idiom to iterate over the
fragments.

Would you send a v2?

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-29  7:24 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26  3:57 [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error Yajun Deng
2021-07-28 16:18 ` Pablo Neira Ayuso
2021-07-29  3:19 ` yajun.deng
2021-07-29  7:24   ` Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).