This looks false positive, probably just needs lockdep_set_class()
to set keys for pipe->mutex and unix->bindlock.
I'm afraid that it's not a false positive at all.
Preparations:
* create an AF_UNIX socket.
* set SOCK_PASSCRED on it.
* create a pipe.
Child 1: splice from pipe to socket; locks pipe and proceeds down towards
unix_dgram_sendmsg().
Child 2: splice from pipe to /mnt/foo/bar; requests write access to /mnt
and blocks on attempt to lock the pipe already locked by (1).
Child 3: freeze /mnt; blocks until (2) is done
Child 4: bind() the socket to /mnt/barf; grabs ->bindlock on the socket and
proceeds to create /mnt/barf, which blocks due to fairness of freezer (no
extra write accesses to something that is in process of being frozen).
_Now_ (1) gets around to unix_dgram_sendmsg(). We still have NULL u->addr,
since bind() has not gotten through yet. We also have SOCK_PASSCRED set,
so we attempt autobind; it blocks on the ->bindlock, which won't be
released until bind() is done (at which point we'll see non-NULL u->addr
and bugger off from autobind), but bind() won't succeed until /mnt
goes through the freeze-thaw cycle, which won't happen until (2) finishes,
which won't happen until (1) unlocks the pipe. Deadlock.
Granted, ->bindlock is taken interruptibly, so it's not that much of
a problem (you can kill the damn thing), but you would need to intervene
and kill it.
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
This looks false positive, probably just needs lockdep_set_class()
to set keys for pipe->mutex and unix->bindlock.
I'm afraid that it's not a false positive at all.
Right, I was totally misled by the scenario output of lockdep, the stack
traces actually are much more reasonable.
The deadlock scenario is easy actually, comparing with the netlink one
which has 4 locks involved, it is:
unix_bind() path:
u->bindlock ==> sb_writer
do_splice() path:
sb_writer ==> pipe->mutex ==> u->bindlock
*** DEADLOCK ***
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-12-09 06:41:44
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
From: Cong Wang <hidden> Date: 2017-01-17 21:22:54
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
Thanks.
On Tue, Jan 17, 2017 at 10:21 PM, Cong Wang [off-list ref] wrote:
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
quoted
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
^
ffff88006b301380: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff88006b301400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
From: Cong Wang <hidden> Date: 2017-01-20 04:57:36
On Wed, Jan 18, 2017 at 1:17 AM, Dmitry Vyukov [off-list ref] wrote:
On Tue, Jan 17, 2017 at 10:21 PM, Cong Wang [off-list ref] wrote:
quoted
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
quoted
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
I instantly hit:
Oh, sorry about it, I forgot to initialize struct path...
Attached is the updated version, I just did a boot test, no crash at least. ;)
Thanks!
On Fri, Jan 20, 2017 at 5:57 AM, Cong Wang [off-list ref] wrote:
quoted
quoted
quoted
quoted
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
I instantly hit:
Oh, sorry about it, I forgot to initialize struct path...
Attached is the updated version, I just did a boot test, no crash at least. ;)
Thanks!
This works! I did not see the deadlock warning, nor any other related crashes.
Tested-by: Dmitry Vyukov <dvyukov@google.com>
On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
quoted
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
I don't think this is the right approach.
Currently the file creation is potponed until unix_bind can no longer
fail otherwise. With it reordered, it may be someone races you with a
different path and now you are left with a file to clean up. Except it
is quite unclear for me if you can unlink it.
I don't have a good idea how to fix it. A somewhat typical approach
would introduce an intermediate state ("under construction") and drop
the lock between calling into unix_mknod.
In this particular case, perhaps you could repurpose gc_flags as a
general flags carrier and add a 'binding in process' flag to test.
From: Cong Wang <hidden> Date: 2017-01-27 05:12:23
On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik [off-list ref] wrote:
On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
quoted
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
quoted
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
I don't think this is the right approach.
Currently the file creation is potponed until unix_bind can no longer
fail otherwise. With it reordered, it may be someone races you with a
different path and now you are left with a file to clean up. Except it
is quite unclear for me if you can unlink it.
What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.
On Thu, Jan 26, 2017 at 09:11:07PM -0800, Cong Wang wrote:
On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik [off-list ref] wrote:
quoted
On Tue, Jan 17, 2017 at 01:21:48PM -0800, Cong Wang wrote:
quoted
On Mon, Jan 16, 2017 at 1:32 AM, Dmitry Vyukov [off-list ref] wrote:
quoted
On Fri, Dec 9, 2016 at 7:41 AM, Al Viro [off-list ref] wrote:
quoted
On Thu, Dec 08, 2016 at 10:32:00PM -0800, Cong Wang wrote:
quoted
quoted
Why do we do autobind there, anyway, and why is it conditional on
SOCK_PASSCRED? Note that e.g. for SOCK_STREAM we can bloody well get
to sending stuff without autobind ever done - just use socketpair()
to create that sucker and we won't be going through the connect()
at all.
In the case Dmitry reported, unix_dgram_sendmsg() calls unix_autobind(),
not SOCK_STREAM.
Yes, I've noticed. What I'm asking is what in there needs autobind triggered
on sendmsg and why doesn't the same need affect the SOCK_STREAM case?
quoted
I guess some lock, perhaps the u->bindlock could be dropped before
acquiring the next one (sb_writer), but I need to double check.
Bad idea, IMO - do you *want* autobind being able to come through while
bind(2) is busy with mknod?
Ping. This is still happening on HEAD.
Thanks for your reminder. Mind to give the attached patch (compile only)
a try? I take another approach to fix this deadlock, which moves the
unix_mknod() out of unix->bindlock. Not sure if there is any unexpected
impact with this way.
I don't think this is the right approach.
Currently the file creation is potponed until unix_bind can no longer
fail otherwise. With it reordered, it may be someone races you with a
different path and now you are left with a file to clean up. Except it
is quite unclear for me if you can unlink it.
What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.
Two threads can race and one fails with EINVAL.
With your patch there is a new file created and it is unclear what to
do with it - leaving it as it is sounds like the last resort and
unlinking it sounds extremely fishy as it opens you to games played by
the user.
From: Cong Wang <hidden> Date: 2017-01-31 06:44:25
On Thu, Jan 26, 2017 at 10:41 PM, Mateusz Guzik [off-list ref] wrote:
On Thu, Jan 26, 2017 at 09:11:07PM -0800, Cong Wang wrote:
quoted
On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik [off-list ref] wrote:
quoted
Currently the file creation is potponed until unix_bind can no longer
fail otherwise. With it reordered, it may be someone races you with a
different path and now you are left with a file to clean up. Except it
is quite unclear for me if you can unlink it.
What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.
Two threads can race and one fails with EINVAL.
With your patch there is a new file created and it is unclear what to
do with it - leaving it as it is sounds like the last resort and
unlinking it sounds extremely fishy as it opens you to games played by
the user.
But the file is created and visible to users too even without my patch,
the file is also put when the unix sock is released. So the only difference
my patch makes is bindlock is no longer taken during file creation, which
does not seem to be the cause of the problem you complain here.
Mind being more specific?
On Mon, Jan 30, 2017 at 10:44:03PM -0800, Cong Wang wrote:
On Thu, Jan 26, 2017 at 10:41 PM, Mateusz Guzik [off-list ref] wrote:
quoted
On Thu, Jan 26, 2017 at 09:11:07PM -0800, Cong Wang wrote:
quoted
On Thu, Jan 26, 2017 at 3:29 PM, Mateusz Guzik [off-list ref] wrote:
quoted
Currently the file creation is potponed until unix_bind can no longer
fail otherwise. With it reordered, it may be someone races you with a
different path and now you are left with a file to clean up. Except it
is quite unclear for me if you can unlink it.
What races do you mean here? If you mean someone could get a
refcount of that file, it could happen no matter we have bindlock or not
since it is visible once created. The filesystem layer should take care of
the file refcount so all we need to do here is calling path_put() as in my
patch. Or if you mean two threads calling unix_bind() could race without
binlock, only one of them should succeed the other one just fails out.
Two threads can race and one fails with EINVAL.
With your patch there is a new file created and it is unclear what to
do with it - leaving it as it is sounds like the last resort and
unlinking it sounds extremely fishy as it opens you to games played by
the user.
But the file is created and visible to users too even without my patch,
the file is also put when the unix sock is released. So the only difference
my patch makes is bindlock is no longer taken during file creation, which
does not seem to be the cause of the problem you complain here.
Mind being more specific?
Consider 2 threads which bind the same socket, but with different paths.
Currently exactly one file will get created, the one used to bind.
With your patch both threads can succeed creating their respective
files, but only one will manage to bind. The other one must error out,
but it already created a file it is unclear what to do with.
From: Cong Wang <hidden> Date: 2017-02-06 07:22:34
On Tue, Jan 31, 2017 at 10:14 AM, Mateusz Guzik [off-list ref] wrote:
On Mon, Jan 30, 2017 at 10:44:03PM -0800, Cong Wang wrote:
quoted
Mind being more specific?
Consider 2 threads which bind the same socket, but with different paths.
Currently exactly one file will get created, the one used to bind.
With your patch both threads can succeed creating their respective
files, but only one will manage to bind. The other one must error out,
but it already created a file it is unclear what to do with.
In this case, it simply puts the path back:
err = -EINVAL;
if (u->addr)
goto out_up;
[...]
out_up:
mutex_unlock(&u->bindlock);
out_put:
if (err)
path_put(&path);
out:
return err;
Which is what unix_release_sock() does too:
if (path.dentry)
path_put(&path);
On Sun, Feb 05, 2017 at 11:22:12PM -0800, Cong Wang wrote:
On Tue, Jan 31, 2017 at 10:14 AM, Mateusz Guzik [off-list ref] wrote:
quoted
On Mon, Jan 30, 2017 at 10:44:03PM -0800, Cong Wang wrote:
quoted
Mind being more specific?
Consider 2 threads which bind the same socket, but with different paths.
Currently exactly one file will get created, the one used to bind.
With your patch both threads can succeed creating their respective
files, but only one will manage to bind. The other one must error out,
but it already created a file it is unclear what to do with.
In this case, it simply puts the path back:
err = -EINVAL;
if (u->addr)
goto out_up;
[...]
out_up:
mutex_unlock(&u->bindlock);
out_put:
if (err)
path_put(&path);
out:
return err;
Which is what unix_release_sock() does too:
if (path.dentry)
path_put(&path);
Yes, but unix_release_sock is expected to leave the file behind.
Note I'm not claiming there is a leak, but that racing threads will be
able to trigger a condition where you create a file and fail to bind it.
What to do with the file now?
Untested, but likely a working solution would rework the code so that
e.g. a flag is set and the lock can be dropped.
From: Cong Wang <hidden> Date: 2017-02-10 01:38:25
On Tue, Feb 7, 2017 at 6:20 AM, Mateusz Guzik [off-list ref] wrote:
Yes, but unix_release_sock is expected to leave the file behind.
Note I'm not claiming there is a leak, but that racing threads will be
able to trigger a condition where you create a file and fail to bind it.
Which is expected, right? No one guarantees the success of file
creation is the success of bind, the previous code does but it is not
part of API AFAIK. Should a sane user-space application check
the file creation for a successful bind() or just check its return value?
What to do with the file now?
We just do what unix_release_sock() does, so why do you keep
asking the same question?
If you still complain about the race with user-space, think about the
same race in-between a successful bind() and close(), nothing is new.