* [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 related [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 related [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).