On Thu, Jul 29, 2021 at 03:19:01AM +0000, yajun.deng@linux.dev wrote:

July 29, 2021 12:18 AM, "Pablo Neira Ayuso" [off-list ref] wrote:

quoted

On Mon, Jul 26, 2021 at 11:57:02AM +0800, Yajun Deng wrote:

quoted

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 <redacted>
---
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.

quoted

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?