Message-ID: <20240516121954.98845-1-kuniyu@amazon.com>
Date: Thu, 16 May 2024 21:19:54 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <mhal@...x.co>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH v5 net 1/2] af_unix: Fix garbage collection of embryos carrying OOB/SCM_RIGHTS.

From: Michal Luczaj <mhal@...x.co>
Date: Thu, 16 May 2024 12:33:35 +0200
> On 5/15/24 15:35, Kuniyuki Iwashima wrote:
> > From: Michal Luczaj <mhal@...x.co>
> > Date: Wed, 15 May 2024 11:34:51 +0200
> >> On 5/15/24 02:32, Kuniyuki Iwashima wrote:
> >>> ...
> >>> The python script below [0] sends a listener's fd to its embryo as OOB
> >>> data.  Then, GC does not iterates the embryo from the listener to drop
> >>> the OOB skb's refcount, and the skb in embryo's receive queue keeps the
> >>> listener's refcount.  As a result, the listener is leaked and the warning
> >>> [1] is hit.
> >>> ...
> >>
> >> Sorry, this does not convey what I wrote. And I think your edit is
> >> incorrect.
> >>
> >> GC starts from the in-flight listener and *does* iterate the embryo; see
> >> scan_children() where scan_inflight() is called for all the embryos.
> > 
> > I meant the current code does not call skb_unref() for embryos's OOB skb
> > because it's done _after_ scan_inflight(), not in scan_inflight().
> 
> Right, I think I see what you mean.
> 
> >> The skb in embryo's RQ *does not* keep the listener's refcount; skb from RQ
> >> ends up in the hit list and is purged.
> > 
> > unix_sk(sk)->oob_skb is a pointer to skb in recvq.  Perhaps I should
> > have written "the skb which was in embryo's receive queue stays as
> > unix_sk(sk)->oob_skb and keeps the listener's refcount".
> 
> I wholeheartedly concur with you!
> 
> >> It is embryo's oob_skb that holds the refcount; see how __unix_gc() goes
> >> over gc_candidates attempting to kfree_skb(u->oob_skb), notice that `u`
> >> here is a listener, not an embryo.
> >>
> >> I understand you're "in rush for the merge window", but would it be okay if
> >> I ask you not to edit my commit messages so heavily?
> > 
> > I noticed the new gc code was merged in Linus' tree.  It's still not
> > synced with net.git, but I guess it will be done soon and your patch
> > will not apply on net.git.  Then, I cannot include your patch as a
> > series, so please feel free to send it to each stable tree.
> 
> All right, no problem. Does it mean you'll be posting PATCH 2/2 ("af_unix:
> Update unix_sk(sk)->oob_skb under sk_receive_queue lock") to stable(s)?

I'll post patch 2/2 to net.git and it will be sent to stable later
by netdev maintainers.  Then, with your patch, the issue is completely
fixed for the old gc.


> 
> Moving on to the New GC: Python test from this patch shows that the New GC
> is memleaking in pretty much the same fashion.

Good catch!

> 
> $ grep splat /proc/net/unix
> $ ./unix-oob-splat.py
> $ rm unix-oob-splat
> $ ./unix-oob-splat.py
> $ grep splat /proc/net/unix
> 0000000000000000: 00000002 00000000 00000000 0001 02     0 unix-oob-splat
> 0000000000000000: 00000002 00000000 00000000 0001 02     0 unix-oob-splat
> 0000000000000000: 00000002 00000000 00010000 0001 01  6643 unix-oob-splat
> 0000000000000000: 00000002 00000000 00010000 0001 01  2920 unix-oob-splat
> 
> I've posted a patch:
> https://lore.kernel.org/netdev/20240516103049.1132040-1-mhal@rbox.co/
> 
> I tried to align with your version of the commit message, but feel free to
> chime in. Also, I took the liberty to introduce a small sanity check. Would
> you prefer if I dropped this hunk or possibly made it a separate patch?

Will comment on the patch thread.

Thanks!

Powered by blists - more mailing lists