Messages in this thread | | | From | Miklos Szeredi <> | Date | Thu, 29 Jul 2021 16:29:01 +0200 | Subject | Re: [PATCH net] af_unix: fix garbage collect vs. MSG_PEEK |
| |
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
| |