On Thu, Sep 1, 2016 at 2:43 PM, Linus Torvalds
[off-list ref] wrote:
On Thu, Sep 1, 2016 at 2:01 PM, Al Viro [off-list ref] wrote:
quoted
Outside as in "all fs activity in bind happens under it". Along with
assignment to ->u.addr, etc. IOW, make it the outermost lock there.
Hah, yes. I misunderstood you.
Yes. In fact that fixes the problem I mentioned, rather than introducing it.
So the easiest approach would seem to be to revert commit c845acb324aa
("af_unix: Fix splice-bind deadlock"), and then apply the lock split.
Like the attached two patches.
This is still *entirely* untested.
Rainer?
Linus
On Thu, Sep 1, 2016 at 2:01 PM, Al Viro [off-list ref] wrote:
quoted
Outside as in "all fs activity in bind happens under it". Along with
assignment to ->u.addr, etc. IOW, make it the outermost lock there.
Hah, yes. I misunderstood you.
Yes. In fact that fixes the problem I mentioned, rather than introducing
it.
So the easiest approach would seem to be to revert commit c845acb324aa
("af_unix: Fix splice-bind deadlock"), and then apply the lock split.
Like the attached two patches.
This is still *entirely* untested.
On Thu, Sep 1, 2016 at 2:01 PM, Al Viro [off-list ref] wrote:
quoted
Outside as in "all fs activity in bind happens under it". Along with
assignment to ->u.addr, etc. IOW, make it the outermost lock there.
Hah, yes. I misunderstood you.
Yes. In fact that fixes the problem I mentioned, rather than introducing
it.
So the easiest approach would seem to be to revert commit c845acb324aa
("af_unix: Fix splice-bind deadlock"), and then apply the lock split.
Like the attached two patches.
This is still *entirely* untested.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-09-02 16:00:36
On Fri, Sep 02, 2016 at 04:18:04PM +0100, Rainer Weikusat wrote:
As far as I can tell, this should work as I can't currently imagine why
a fs operation might end up binding a unix socket despite the idea to
make af_unix.c yet more complicated in order to work around irregular
behaviour of (as far as I can tell) a single filesystem (for which
kern_path_create doesn't really mean kern_path_create
Bullshit. kern_path_create() *does* mean the same thing in all cases.
Namely, find the parent, lock it and leave the final name component for
the create-type operation. It sure as hell is not guaranteed to take
*all* locks that are going to be taken in process of mknod/mkdir/etc.
Never had been.
and it has to work
around that once it gets control) goes against all instincts I have in
this area. If filesystems need to do arbitrary stuff when
__sb_start_write is called for 'their' superblock, they should be able
to do so directly.
At present, this is a theoretic concern as I can't (due to other work
committments) put any non-cursory work into this before Sunday. There
may also be other reasons why this idea is impractical or even
unworkable.
On Fri, Sep 02, 2016 at 04:18:04PM +0100, Rainer Weikusat wrote:
quoted
As far as I can tell, this should work as I can't currently imagine
why a fs operation might end up binding a unix socket despite the
idea to make af_unix.c yet more complicated in order to work around
irregular behaviour of (as far as I can tell) a single filesystem
(for which kern_path_create doesn't really mean kern_path_create and
it has to work around that once it gets control) goes against all
instincts I have in this area. If filesystems need to do arbitrary
stuff when __sb_start_write is called for 'their' superblock, they
should be able to do so directly.
Bullshit. kern_path_create() *does* mean the same thing in all cases.
Namely, find the parent, lock it and leave the final name component for
the create-type operation. It sure as hell is not guaranteed to take
*all* locks that are going to be taken in process of mknod/mkdir/etc.
Never had been.
This isn't about "all locks", it's about the lock in question. No other
mknod operation (I'm aware of) calls this with another superblock than
the one already acted upon by kern_path_create. This may be wrong (if
so, feel free to correct it) but it's not "bullshit" (intentional
deception in order to sell something to someone).
On Thu, Sep 1, 2016 at 2:01 PM, Al Viro [off-list ref]
wrote:
quoted
Outside as in "all fs activity in bind happens under it". Along with
assignment to ->u.addr, etc. IOW, make it the outermost lock there.
Hah, yes. I misunderstood you.
Yes. In fact that fixes the problem I mentioned, rather than
introducing
it.
So the easiest approach would seem to be to revert commit c845acb324aa
("af_unix: Fix splice-bind deadlock"), and then apply the lock split.
Like the attached two patches.
This is still *entirely* untested.
Tested-by: CAI Qian <redacted>
OK, this tag still stand. The below issue is also reproduced without those
patches, so a separate problem most likely was introduced recently (after
rc3 or rc4) by probably some xfs update.
CAI Qian
On Thu, Sep 1, 2016 at 2:43 PM, Linus Torvalds
[off-list ref] wrote:
quoted
On Thu, Sep 1, 2016 at 2:01 PM, Al Viro [off-list ref] wrote:
quoted
Outside as in "all fs activity in bind happens under it". Along with
assignment to ->u.addr, etc. IOW, make it the outermost lock there.
Hah, yes. I misunderstood you.
Yes. In fact that fixes the problem I mentioned, rather than introducing it.
So the easiest approach would seem to be to revert commit c845acb324aa
("af_unix: Fix splice-bind deadlock"), and then apply the lock split.
Like the attached two patches.
This is still *entirely* untested.
As far as I can tell, this should work as I can't currently imagine why
a fs operation might end up binding a unix socket despite the idea to
make af_unix.c yet more complicated in order to work around irregular
behaviour of (as far as I can tell) a single filesystem (for which
kern_path_create doesn't really mean kern_path_create and it has to work
around that once it gets control) goes against all instincts I have in
this area. If filesystems need to do arbitrary stuff when
__sb_start_write is called for 'their' superblock, they should be able
to do so directly.
At present, this is a theoretic concern as I can't (due to other work
committments) put any non-cursory work into this before Sunday. There
may also be other reasons why this idea is impractical or even
unworkable.
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-09-02 17:03:03
On Fri, Sep 02, 2016 at 05:10:13PM +0100, Rainer Weikusat wrote:
quoted
Bullshit. kern_path_create() *does* mean the same thing in all cases.
Namely, find the parent, lock it and leave the final name component for
the create-type operation. It sure as hell is not guaranteed to take
*all* locks that are going to be taken in process of mknod/mkdir/etc.
Never had been.
This isn't about "all locks", it's about the lock in question. No other
mknod operation (I'm aware of) calls this with another superblock than
the one already acted upon by kern_path_create. This may be wrong (if
so, feel free to correct it) but it's not "bullshit" (intentional
deception in order to sell something to someone).
Never had been promised. And it's not just this lock - e.g. ->i_rwsem is
taken on the parent by kern_path_create() and on parent in underlying
filesystem by ecryptfs ->mknod() (as well as overlayfs one). bind/bind
deadlock - one for a path to ecryptfs, another for that on the raw
filesystem behind it (which can be mounted elsewhere/in another namespace/etc.)
with those paths ending in the matching directories (the last components may
be same or different - doesn't matter)
A: lock parent in ecryptfs (via kern_path_create())
B: lock the directory behind it in underlying fs (ditto)
A: grab ->readlock
B: block on ->readlock
A: call ecryptfs_mknod() and block trying to lock the directory held by B
Deadlock. And while we are at it, ecryptfs probably ought to claim transient
write access for the duration of modifications of the underlying one similar
to overlayfs. The point is, it had never been promised that you can stick
random locks just outside of ->mknod()/->mkdir()/etc. The same goes for e.g.
NFS mount of something exported by localhost; knfsd must lock the parent
directory on server before creating anything in it. Suppose you have
/srv/nfs/foo exported and mounted on the same host at /mnt/bar. bind to
/mnt/bar/barf/a vs. bind to /srv/nfs/foo/barf/b:
A: lock /mnt/bar/barf
B: lock /srv/nfs/foo/barf
A: grab ->readlock
B: block on ->readlock
A: call nfs_mknod(), wait for reply
knfsd: block trying to lock /srv/nfs/foo/barf
It's very much _not_ just overlayfs being pathological - that it certainly is,
but the problem is much wider. You could try to argue that kern_path_create()
should've known to lock all relevant directories in case of overlayfs and
ecryptfs, but it has absolutely no chance to do that in case of NFS - the
protocol doesn't allow "lock this directory, one of my next requests will
be to create something in it". Even leases are only for regular files...
This is a different deadlock, though. This is a deadlock due to mixed
lock ordering between the pipe mutex, the XFS "mr_lock", and the inode
mutex.
If I read the lockdep trace correctly, we have:
Normally we have write doing inode->i_mutex -> i_iolock.mr_lock fro
the regular write path.
But the normal splice "write()" case has
pipe->mutex -> filesystem write lock (normally i_mutex)
(in iter_file_splice_write() that calls vfs_iter_write() that calls
->write_iter())
and then the XFS splice case as
mr_lock -> pipe->mutex
in xfs_file_splice_read() calling splice_to_pipe().
So you end up with a A->B->C->A chain.
I think it's new to the new iomap based buffered write path in 4.8.
Dave, Christoph?
Linus
On Fri, Sep 2, 2016 at 10:02 AM, Al Viro [off-list ref] wrote:
It's very much _not_ just overlayfs being pathological - that it certainly is,
but the problem is much wider.
Al, can you take a look at my two patches, and see if you agree that
they fix it, though?
Of course, we now have *another* splice deadlock. That pipe inode is
nasty, it's very easy to deadlock on it in subtle ways.
Linus
From: Al Viro <viro@ZenIV.linux.org.uk> Date: 2016-09-02 17:52:14
On Fri, Sep 02, 2016 at 10:12:08AM -0700, Linus Torvalds wrote:
On Fri, Sep 2, 2016 at 10:02 AM, Al Viro [off-list ref] wrote:
quoted
It's very much _not_ just overlayfs being pathological - that it certainly is,
but the problem is much wider.
Al, can you take a look at my two patches, and see if you agree that
they fix it, though?
AFAICS, they should. Locking is obviously saner that way and AFAICS the
rest is absolutely straightforward.
Acked-by: Al Viro <viro@zeniv.linux.org.uk>
Of course, we now have *another* splice deadlock. That pipe inode is
nasty, it's very easy to deadlock on it in subtle ways.
I'm still digging through iomap.c, but that's better taken to another branch
of this thread...
On Fri, Sep 2, 2016 at 10:02 AM, Al Viro [off-list ref] wrote:
quoted
It's very much _not_ just overlayfs being pathological - that it certainly is,
but the problem is much wider.
Al, can you take a look at my two patches, and see if you agree that
they fix it, though?
The original deadlock occurred because of some code path locking the
superblock followed by trying to acquire the af_unix readlock while
unix_bind did the same in the opposite order (by doing kern_path_create
with the readlock held). If unix_bind doesn't share a lock with the
receive routines anymore, this obviously can't happen anymore.
The other problem situation is one where a lock on someting can be
acquired both by kern_path_create and a mknod operation and the readlock
is taken in between. Because that sits in between the kern_path_create
and the mknod, it can block the thread which got a certain lock via
kern_path_create because the one which is about to try to acquire it via
mknod got the readlock first. This obviously can't happen anymore the when the
original 'acquire readlock (now bindlock) first' is restored.