Hi Cong,
No, I have never ignored any comment from reviewers. I sent v2 on Oct 26 after
discussing with Xin Long, and v3 on Oct 27 after receiving comment from Jakub.
I received your 3 emails nearly at the same time on Oct 28. It's weird. Your
emails did not appear in this email archive either:
https://sourceforge.net/p/tipc/mailman/tipc-discussion/
Anyway, I answer your questions:
1/ Why it is not correct if not decreasing the data reference counter in
tipc_buf_append()
In my changelog, I just pinpointed the place where the leak would happen. I
show you the details here:
tipc_msg_reassemble(list,-)
{
...
frag = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the
original skb has the value of 2.
...
If (tipc_buf_append(&head, &frag)) // each data reference counter of the
original skb STILL has the value of 2 because the usage of skb_copy() instead
of skb_unshare()
...
}
The original skb list then is passed to tipc_bcast_xmit() which in turn calls
tipc_link_xmit():
tipc_link_xmit(-, list, -)
{
...
_skb = skb_clone(skb, GFP_ATOMIC); // each data reference counter of the
original skb has the value of 3.
...
}
When each cloned skb is sent out by the driver, it is freed by the driver. That
leads to each data reference counter of the original skb has the value of 2.
After receiving ACK from another peer, the original skb needs to be freed:
tipc_link_advance_transmq()
{
...
kfree_skb(skb); // memory exists after being freed because the data reference
counter still has the value of 2.
}
This indeed causes memory leak.
2/ Why previously-used skb_unclone() works.
The purpose of skb_unclone() is to unclone the cloned skb. So, it does not make
any sense to say that " skb_unclone() expects refcnt == 1" as I understand
you implied the data reference counter.
pskb_expand_head() inside skb_unclone() requires that the user reference
counter has the value of 1 as implemented:
pskb_expand_head()
{
...
BUG_ON(skb_shared(skb)); // User reference counter must be 1
...
atomic_set(&skb_shinfo(skb)->dataref, 1); // The data reference counter of the
original skb has the value of 1
...
}
That explains why after being passed to tipc_link_xmit(), each data reference
counter of each original skb has the value of 2 and can be freed in
tipc_link_advance_transmq().
Best regards,
Tung Nguyen
-----Original Message-----
From: Cong Wang <xiyou.wangc...@gmail.com>
Sent: Wednesday, October 28, 2020 3:50 AM
To: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>
Cc: David Miller <da...@davemloft.net>; Linux Kernel Network Developers
<netdev@vger.kernel.org>; tipc-discuss...@lists.sourceforge.net
Subject: Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by
tipc_buf_append()
On Tue, Oct 27, 2020 at 1:09 PM Tung Nguyen
<tung.q.ngu...@dektech.com.au> wrote:
>
> Commit ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> replaced skb_unshare() with skb_copy() to not reduce the data reference
> counter of the original skb intentionally. This is not the correct
> way to handle the cloned skb because it causes memory leak in 2
> following cases:
> 1/ Sending multicast messages via broadcast link
> The original skb list is cloned to the local skb list for local
> destination. After that, the data reference counter of each skb
> in the original list has the value of 2. This causes each skb not
> to be freed after receiving ACK:
This does not make sense at all.
skb_unclone() expects refcnt == 1, as stated in the comments
above pskb_expand_head(). skb_unclone() was used prior to
Xin Long's commit.
So either the above is wrong, or something important is still missing
in your changelog. None of them is addressed in your V3.
I also asked you two questions before you sent V3, you seem to
intentionally ignore them. This is not how we collaborate.
Thanks.
