From: Miklos Szeredi <mszeredi@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
netdev@vger.kernel.org, lkml <linux-kernel@vger.kernel.org>,
David Miller <davem@davemloft.net>,
kuba@kernel.org, stable <stable@vger.kernel.org>
Subject: Re: [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK
Date: Thu, 29 Jul 2021 16:29:01 +0200 [thread overview]
Message-ID: <CAOssrKdjuUbyHmzwLJFtu-KibPgG3s=LoDq3fgzkv=kTG+mZiQ@mail.gmail.com> (raw)
In-Reply-To: <202107261049.DC0C9178@keescook>
On Mon, Jul 26, 2021 at 9:27 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jul 26, 2021 at 05:36:21PM +0200, Greg Kroah-Hartman wrote:
> > From: Miklos Szeredi <mszeredi@redhat.com>
> >
> > Gc assumes that in-flight sockets that don't have an external ref can't
>
> I think this commit log could be expanded. I had to really study things
> to even beging to understand what was going on. I assume "Gc" here means
> specifically unix_gc()?
Yeah, the original description was not too good. Commit cbcf01128d0a
("af_unix: fix garbage collect vs MSG_PEEK") now in Linus' tree has a
much expanded description.
> I note that unix_tot_inflight isn't an atomic but is read outside of
> locking by unix_release_sock() and wait_for_unix_gc(), which seems wrong
> (or at least inefficient).
I don't think it matters in practice. Do you have specific worries?
> Doesn't this mean total_refs and inflight_refs can still get out of
> sync? What keeps an skb from being "visible" to unix_peek_fds() between
> the unix_gx() spin_unlock() and the unix_peek_fds() fget()?
>
> A: unix_gx():
> spin_lock()
> find "total_refs == inflight_refs", add to hitlist
> spin_unlock()
> B: unix_peek_fds():
> fget()
> A: unix_gc():
> walk hitlist and free(skb)
> B: unix_peek_fds():
> *use freed skb*
>
> I feel like I must be missing something since the above race would
> appear to exist even for unix_attach_fds()/unix_detach_fds():
What you are missing is that anything that could have been peeked must
not have been garbage collected. I.e. the garbage collection
algorithm will find that there's an external in-flight reference to
the peeked socket and so it will not add it to the hitlist.
Thanks,
Miklos
prev parent reply other threads:[~2021-07-29 14:29 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-26 15:36 [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK Greg Kroah-Hartman
2021-07-26 19:27 ` Kees Cook
2021-07-29 14:29 ` Miklos Szeredi [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='CAOssrKdjuUbyHmzwLJFtu-KibPgG3s=LoDq3fgzkv=kTG+mZiQ@mail.gmail.com' \
--to=mszeredi@redhat.com \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=stable@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.