From: Xin Long <lucien.xin@gmail.com>
To: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Cc: davem <davem@davemloft.net>, network dev <netdev@vger.kernel.org>,
tipc-discussion@lists.sourceforge.net
Subject: Re: [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append()
Date: Wed, 28 Oct 2020 13:21:17 +0800 [thread overview]
Message-ID: <CADvbK_fPKYoyk1w4ri_oxRiutGZDE9FaE5xrx1UXgY2_gdCO0Q@mail.gmail.com> (raw)
In-Reply-To: <20201027032403.1823-1-tung.q.nguyen@dektech.com.au>
On Tue, Oct 27, 2020 at 11:25 AM Tung Nguyen
<tung.q.nguyen@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:
> tipc_link_advance_transmq()
> {
> ...
> /* release skb */
> __skb_unlink(skb, &l->transmq);
> kfree_skb(skb); <-- memory exists after being freed
> }
>
> 2/ Sending multicast messages via replicast link
> Similar to the above case, each skb cannot be freed after purging
> the skb list:
> tipc_mcast_xmit()
> {
> ...
> __skb_queue_purge(pkts); <-- memory exists after being freed
> }
>
> This commit fixes this issue by using skb_unshare() instead. Besides,
> to avoid use-after-free error reported by KASAN, the pointer to the
> fragment is set to NULL before calling skb_unshare() to make sure that
> the original skb is not freed after freeing the fragment 2 times in
> case skb_unshare() returns NULL.
>
> Fixes: ed42989eab57 ("tipc: fix the skb_unshare() in tipc_buf_append()")
> Acked-by: Jon Maloy <jmaloy@redhat.com>
> Reported-by: Thang Hoang Ngo <thang.h.ngo@dektech.com.au>
> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Reviewed-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/tipc/msg.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 2a78aa701572..32c79c59052b 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -150,12 +150,11 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
> if (fragid == FIRST_FRAGMENT) {
> if (unlikely(head))
> goto err;
> - if (skb_cloned(frag))
> - frag = skb_copy(frag, GFP_ATOMIC);
> + *buf = NULL;
> + frag = skb_unshare(frag, GFP_ATOMIC);
> if (unlikely(!frag))
> goto err;
> head = *headbuf = frag;
> - *buf = NULL;
> TIPC_SKB_CB(head)->tail = NULL;
> if (skb_is_nonlinear(head)) {
> skb_walk_frags(head, tail) {
> --
> 2.17.1
>
>
>
> _______________________________________________
> tipc-discussion mailing list
> tipc-discussion@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
next prev parent reply other threads:[~2020-10-28 22:11 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-27 3:24 [tipc-discussion] [net v3 1/1] tipc: fix memory leak caused by tipc_buf_append() Tung Nguyen
2020-10-27 20:50 ` Cong Wang
2020-10-28 5:23 ` Tung Quang Nguyen
2020-10-28 19:29 ` Cong Wang
2020-10-28 5:21 ` Xin Long [this message]
2020-10-28 19:31 ` Cong Wang
2020-10-29 16:55 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADvbK_fPKYoyk1w4ri_oxRiutGZDE9FaE5xrx1UXgY2_gdCO0Q@mail.gmail.com \
--to=lucien.xin@gmail.com \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
--cc=tipc-discussion@lists.sourceforge.net \
--cc=tung.q.nguyen@dektech.com.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).