From: Michal Luczaj <mhal@rbox.co>
To: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: billy@starlabs.sg, davem@davemloft.net, edumazet@google.com,
kuba@kernel.org, kuni1840@gmail.com, netdev@vger.kernel.org,
pabeni@redhat.com
Subject: Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.
Date: Wed, 15 May 2024 11:28:49 +0200 [thread overview]
Message-ID: <b11c0bdf-a7ea-4fdf-b07c-3034229aaa5f@rbox.co> (raw)
In-Reply-To: <20240515000709.41004-1-kuniyu@amazon.com>
On 5/15/24 02:07, Kuniyuki Iwashima wrote:
> From: Michal Luczaj <mhal@rbox.co>
> Date: Tue, 14 May 2024 12:13:36 +0200
>> On 5/14/24 04:52, Kuniyuki Iwashima wrote:
>>> ...
>>> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
>>> index 0104be9d4704..b87e48e2b51b 100644
>>> --- a/net/unix/garbage.c
>>> +++ b/net/unix/garbage.c
>>> @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work)
>>> scan_children(&u->sk, inc_inflight, &hitlist);
>>>
>>> #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>>> + spin_lock(&u->sk.sk_receive_queue.lock);
>>> if (u->oob_skb) {
>>> - kfree_skb(u->oob_skb);
>>> + WARN_ON_ONCE(skb_unref(u->oob_skb));
>>> u->oob_skb = NULL;
>>> }
>>> + spin_unlock(&u->sk.sk_receive_queue.lock);
>>> #endif
>>> }
>>
>> I've realised this part of GC is broken for embryos. And adding a rq lock
>> here turns a warning into a possible deadlock, so below is my attempt at
>> fixing the underlying problem.
>
> Exactly, I missed that case. It's memleak rather than deadlock.
>
> We need to traverse embryos from listener to drop OOB skb refcount
> in embroy recvq to drop listener fd's refcount.
In a way, yeah. See below.
>> It's based it on top of your patch, so should I post it now or wait until
>> your patch lands in net?
>
> I'll post your patch within v5 that will minimise the delay given
> we are in rush for the merge window.
Awesome!
>> Subject: [PATCH] af_unix: Fix garbage collection of embryos carrying
>> OOB/SCM_RIGHTS
>>
>> GC attempts to explicitly drop oob_skb before purging the hit list. The
>
> s/oob_skb/oob_skb's refcount/
Ah, yeah, you're right.
>> problem is with embryos: instead of trying to kfree_skb(u->oob_skb) of an
>> embryo socket, GC goes for its parent-listener socket, which never carries
>> u->oob_skb. Effectively oob_skb is removed from the receive queue, but
>> remains reachable via u->oob_skb.
>
> The last sentence is not correct as the listener does not have oob_skb and
> kfree_skb() is not called.
I was referring to embryo's oob_skb. Anyway, I took a look at your v5
series and I see you've changed my commit message in ways I disagree
with, so I'll comment there.
> I'll post this patch with some modification of commit message.
>
> Thanks!
prev parent reply other threads:[~2024-05-15 9:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-14 2:52 [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock Kuniyuki Iwashima
2024-05-14 10:13 ` Michal Luczaj
2024-05-15 0:07 ` Kuniyuki Iwashima
2024-05-15 9:28 ` Michal Luczaj [this message]
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=b11c0bdf-a7ea-4fdf-b07c-3034229aaa5f@rbox.co \
--to=mhal@rbox.co \
--cc=billy@starlabs.sg \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=kuni1840@gmail.com \
--cc=kuniyu@amazon.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).