| Message ID | 20210726035702.11964-1-yajun.deng@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | netfilter: nf_conntrack_bridge: Fix not free when error | expand |
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 > 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:
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
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?
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.
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(-)