* [Bridge] [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error
@ 2021-07-26 3:57 ` Yajun Deng
0 siblings, 0 replies; 8+ messages in thread
From: Yajun Deng @ 2021-07-26 3:57 UTC (permalink / raw)
To: pablo, kadlec, fw, roopa, nikolay, davem, kuba
Cc: netdev, bridge, linux-kernel, coreteam, Yajun Deng,
netfilter-devel
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 related [flat|nested] 8+ messages in thread* [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error @ 2021-07-26 3:57 ` Yajun Deng 0 siblings, 0 replies; 8+ 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 related [flat|nested] 8+ messages in thread
* Re: [Bridge] [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 -1 siblings, 0 replies; 8+ messages in thread From: Pablo Neira Ayuso @ 2021-07-28 16:18 UTC (permalink / raw) To: Yajun Deng Cc: netdev, bridge, fw, linux-kernel, kadlec, coreteam, netfilter-devel, nikolay, roopa, kuba, davem [-- 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 related [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error @ 2021-07-28 16:18 ` Pablo Neira Ayuso 0 siblings, 0 replies; 8+ 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 related [flat|nested] 8+ messages in thread
* Re: [Bridge] [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error 2021-07-28 16:18 ` Pablo Neira Ayuso @ 2021-07-29 3:19 ` yajun.deng -1 siblings, 0 replies; 8+ messages in thread From: yajun.deng @ 2021-07-29 3:19 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: netdev, bridge, fw, linux-kernel, kadlec, coreteam, netfilter-devel, nikolay, roopa, kuba, davem 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] 8+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error @ 2021-07-29 3:19 ` yajun.deng 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [Bridge] [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 -1 siblings, 0 replies; 8+ messages in thread From: Pablo Neira Ayuso @ 2021-07-29 7:24 UTC (permalink / raw) To: yajun.deng Cc: netdev, bridge, fw, linux-kernel, kadlec, coreteam, netfilter-devel, nikolay, roopa, kuba, davem 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] 8+ messages in thread
* Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error @ 2021-07-29 7:24 ` Pablo Neira Ayuso 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
end of thread, other threads:[~2021-07-29 7:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-07-26 3:57 [Bridge] [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error Yajun Deng 2021-07-26 3:57 ` Yajun Deng 2021-07-28 16:18 ` [Bridge] " Pablo Neira Ayuso 2021-07-28 16:18 ` Pablo Neira Ayuso 2021-07-29 3:19 ` [Bridge] " yajun.deng 2021-07-29 3:19 ` yajun.deng 2021-07-29 7:24 ` [Bridge] " Pablo Neira Ayuso 2021-07-29 7:24 ` Pablo Neira Ayuso
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.