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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
July 29, 2021 12:18 AM, "Pablo Neira Ayuso" <[email protected]> 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 <[email protected]>
>> ---
>> 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, [email protected] wrote:
> July 29, 2021 12:18 AM, "Pablo Neira Ayuso" <[email protected]> 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 <[email protected]>
> >> ---
> >> 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?