vfs: Fix potential circular locking through setxattr() and removexattr()

When using cachefiles, lockdep may emit something similar to the circular locking dependency notice below. The problem appears to stem from the following:

(1) Cachefiles manipulates xattrs on the files in its cache when called

     from ->writepages().

(2) The setxattr() and removexattr() system call handlers get the name (and value) from userspace after taking the sb_writers lock, putting accesses of the vma->vm_lock and mm->mmap_lock inside of that.

(3) The afs filesystem uses a per-inode lock to prevent multiple revalidation RPCs and in writeback vs truncate to prevent parallel operations from deadlocking against the server on one side and local page locks on the other.

Fix this by moving the getting of the name and value in {get,remove}xattr() outside of the sb_writers lock. This also has the minor benefits that we don't need to reget these in the event of a retry and we never try to take the sb_writers lock in the event we can't pull the name and value into the kernel.

======================================================
WARNING: possible circular locking dependency detected
6.10.0-build2+ #956 Not tainted
------------------------------------------------------
fsstress/6050 is trying to acquire lock:
ffff888138fd82f0 (mapping.invalidate_lock#3){++++}-{3:3}, at: filemap_fault+0x26e/0x8b0

but task is already holding lock:
ffff888113f26d18 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x165/0x250

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #4 (&vma->vm_lock->lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_write+0x3b/0x50
        vma_start_write+0x6b/0xa0
        vma_link+0xcc/0x140
        insert_vm_struct+0xb7/0xf0
        alloc_bprm+0x2c1/0x390
        kernel_execve+0x65/0x1a0
        call_usermodehelper_exec_async+0x14d/0x190
        ret_from_fork+0x24/0x40
        ret_from_fork_asm+0x1a/0x30

-> #3 (&mm->mmap_lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        __might_fault+0x7c/0xb0
        strncpy_from_user+0x25/0x160
        removexattr+0x7f/0x100
        __do_sys_fremovexattr+0x7e/0xb0
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #2 (sb_writers#14){.+.+}-{0:0}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        percpu_down_read+0x3c/0x90
        vfs_iocb_iter_write+0xe9/0x1d0
        __cachefiles_write+0x367/0x430
        cachefiles_issue_write+0x299/0x2f0
        netfs_advance_write+0x117/0x140
        netfs_write_folio.isra.0+0x5ca/0x6e0
        netfs_writepages+0x230/0x2f0
        afs_writepages+0x4d/0x70
        do_writepages+0x1e8/0x3e0

        filemap_fdatawrite_wbc+0x84/0xa0
        __filemap_fdatawrite_range+0xa8/0xf0
        file_write_and_wait_range+0x59/0x90
        afs_release+0x10f/0x270
        __fput+0x25f/0x3d0
        __do_sys_close+0x43/0x70
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #1 (&vnode->validate_lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_read+0x95/0x200
        afs_writepages+0x37/0x70
        do_writepages+0x1e8/0x3e0
        filemap_fdatawrite_wbc+0x84/0xa0
        filemap_invalidate_inode+0x167/0x1e0
        netfs_unbuffered_write_iter+0x1bd/0x2d0
        vfs_write+0x22e/0x320
        ksys_write+0xbc/0x130
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #0 (mapping.invalidate_lock#3){++++}-{3:3}:

        check_noncircular+0x119/0x160
        check_prev_add+0x195/0x430
        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_read+0x95/0x200
        filemap_fault+0x26e/0x8b0
        __do_fault+0x57/0xd0
        do_pte_missing+0x23b/0x320
        __handle_mm_fault+0x2d4/0x320
        handle_mm_fault+0x14f/0x260
        do_user_addr_fault+0x2a2/0x500
        exc_page_fault+0x71/0x90
        asm_exc_page_fault+0x22/0x30

Quoted 16 lines · David Howells

When using cachefiles, lockdep may emit something similar to the circular locking dependency notice below. The problem appears to stem from the following:

(1) Cachefiles manipulates xattrs on the files in its cache when called

     from ->writepages().

(2) The setxattr() and removexattr() system call handlers get the name (and value) from userspace after taking the sb_writers lock, putting accesses of the vma->vm_lock and mm->mmap_lock inside of that.

(3) The afs filesystem uses a per-inode lock to prevent multiple revalidation RPCs and in writeback vs truncate to prevent parallel operations from deadlocking against the server on one side and local page locks on the other.

Quoted 5 lines

Fix this by moving the getting of the name and value in {get,remove}xattr() outside of the sb_writers lock. This also has the minor benefits that we don't need to reget these in the event of a retry and we never try to take the sb_writers lock in the event we can't pull the name and value into the kernel.

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is

created, in particular write(2) path is a place where it would be very difficult to get rid of it (you take sb_writers, then do all the work preparing the write and then you copy user data into page cache which may require mmap_sem).

But looking at the lockdep splat below:

Quoted 49 lines

======================================================
WARNING: possible circular locking dependency detected
6.10.0-build2+ #956 Not tainted
------------------------------------------------------
fsstress/6050 is trying to acquire lock:
ffff888138fd82f0 (mapping.invalidate_lock#3){++++}-{3:3}, at: filemap_fault+0x26e/0x8b0

but task is already holding lock:
ffff888113f26d18 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x165/0x250

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #4 (&vma->vm_lock->lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_write+0x3b/0x50
        vma_start_write+0x6b/0xa0
        vma_link+0xcc/0x140
        insert_vm_struct+0xb7/0xf0
        alloc_bprm+0x2c1/0x390
        kernel_execve+0x65/0x1a0
        call_usermodehelper_exec_async+0x14d/0x190
        ret_from_fork+0x24/0x40
        ret_from_fork_asm+0x1a/0x30

-> #3 (&mm->mmap_lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        __might_fault+0x7c/0xb0
        strncpy_from_user+0x25/0x160
        removexattr+0x7f/0x100
        __do_sys_fremovexattr+0x7e/0xb0
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #2 (sb_writers#14){.+.+}-{0:0}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        percpu_down_read+0x3c/0x90
        vfs_iocb_iter_write+0xe9/0x1d0
        __cachefiles_write+0x367/0x430
        cachefiles_issue_write+0x299/0x2f0
        netfs_advance_write+0x117/0x140
        netfs_write_folio.isra.0+0x5ca/0x6e0
        netfs_writepages+0x230/0x2f0
        afs_writepages+0x4d/0x70
        do_writepages+0x1e8/0x3e0

Quoted 8 lines

        filemap_fdatawrite_wbc+0x84/0xa0
        __filemap_fdatawrite_range+0xa8/0xf0
        file_write_and_wait_range+0x59/0x90
        afs_release+0x10f/0x270
        __fput+0x25f/0x3d0
        __do_sys_close+0x43/0x70
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

Quoted 15 lines

-> #1 (&vnode->validate_lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_read+0x95/0x200
        afs_writepages+0x37/0x70
        do_writepages+0x1e8/0x3e0
        filemap_fdatawrite_wbc+0x84/0xa0
        filemap_invalidate_inode+0x167/0x1e0
        netfs_unbuffered_write_iter+0x1bd/0x2d0
        vfs_write+0x22e/0x320
        ksys_write+0xbc/0x130
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #0 (mapping.invalidate_lock#3){++++}-{3:3}:

Quoted 13 lines

        check_noncircular+0x119/0x160
        check_prev_add+0x195/0x430
        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_read+0x95/0x200
        filemap_fault+0x26e/0x8b0
        __do_fault+0x57/0xd0
        do_pte_missing+0x23b/0x320
        __handle_mm_fault+0x2d4/0x320
        handle_mm_fault+0x14f/0x260
        do_user_addr_fault+0x2a2/0x500
        exc_page_fault+0x71/0x90
        asm_exc_page_fault+0x22/0x30

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 23 lines · Jan Kara

Quoted 21 lines · David Howells

When using cachefiles, lockdep may emit something similar to the circular locking dependency notice below. The problem appears to stem from the following:

(1) Cachefiles manipulates xattrs on the files in its cache when called

     from ->writepages().

(2) The setxattr() and removexattr() system call handlers get the name (and value) from userspace after taking the sb_writers lock, putting accesses of the vma->vm_lock and mm->mmap_lock inside of that.

(3) The afs filesystem uses a per-inode lock to prevent multiple revalidation RPCs and in writeback vs truncate to prevent parallel operations from deadlocking against the server on one side and local page locks on the other.

Fix this by moving the getting of the name and value in {get,remove}xattr() outside of the sb_writers lock. This also has the minor benefits that we don't need to reget these in the event of a retry and we never try to take the sb_writers lock in the event we can't pull the name and value into the kernel.

Quoted 2 lines

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is

Independent of this issue, I think that moving the retrieval of name and value out of the lock is a good thing. The commit message might need to get reworded of course.

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is created, in particular write(2) path is a place where it would be very difficult to get rid of it (you take sb_writers, then do all the work preparing the write and then you copy user data into page cache which may require mmap_sem).

...

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

-> #2 (sb_writers#14){.+.+}-{0:0}:

Quoted 9 lines · Jan Kara

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is created, in particular write(2) path is a place where it would be very difficult to get rid of it (you take sb_writers, then do all the work preparing the write and then you copy user data into page cache which may require mmap_sem).

...

Quoted 8 lines

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

I'm not sure you're correct about that. If you look at the lockdep splat:

Quoted 1 line

-> #2 (sb_writers#14){.+.+}-{0:0}:

The sb_writers lock is "personalised" to the filesystem type (the "#14" annotation) which is set here:

	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
		if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
					sb_writers_name[i],
					&type->s_writers_key[i]))  <----
			goto fail;
	}

in fs/super.c.

I think the problem is (1) that on one side, you've got, say, sys_setxattr() taking an sb_writers lock and then accessing a userspace buffer, which (a) may take mm->mmap_lock and vma->vm_lock and (b) may cause reading or writeback from the netfs-based filesystem via an mmapped xattr name buffer].

Then (2) on the other side, you have a read or a write to the network filesystem through netfslib which may invoke the cache, which may require cachefiles to check the xattr on the cache file and maybe set/remove it - which requires the sb_writers lock on the cache filesystem.

So if ->read_folio(), ->readahead() or ->writepages() can ever be called with mm->mmap_lock or vma->vm_lock held, netfslib may call down to cachefiles and ultimately, it should[*] then take the sb_writers lock on the backing filesystem to perform xattr manipulation.

[*] I say "should" because at the moment cachefiles calls vfs_set/removexattr
functions which *don't* take this lock (which is a bug). Is this an error on the part of vfs_set/removexattr()? Should they take this lock analogously with vfs_truncate() and vfs_iocb_iter_write()?

However, as it doesn't it manages to construct a locking chain via the mapping.invalidate_lock, the afs vnode->validate_lock and something in execve that I don't exactly follow.

I wonder if this is might be deadlockable by a multithreaded process (ie. so they share the mm locks) where one thread is writing to a cached file whilst another thread is trying to set/remove the xattr on that file.

David

Quoted 9 lines · Jan Kara

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is created, in particular write(2) path is a place where it would be very difficult to get rid of it (you take sb_writers, then do all the work preparing the write and then you copy user data into page cache which may require mmap_sem).

...

Quoted 8 lines

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

I'm not sure you're correct about that. If you look at the lockdep splat:

Quoted 1 line

-> #2 (sb_writers#14){.+.+}-{0:0}:

The sb_writers lock is "personalised" to the filesystem type (the "#14" annotation) which is set here:

	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
		if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
					sb_writers_name[i],
					&type->s_writers_key[i]))  <----
			goto fail;
	}

in fs/super.c.

I think the problem is (1) that on one side, you've got, say, sys_setxattr() taking an sb_writers lock and then accessing a userspace buffer, which (a) may take mm->mmap_lock and vma->vm_lock and (b) may cause reading or writeback from the netfs-based filesystem via an mmapped xattr name buffer].

Then (2) on the other side, you have a read or a write to the network filesystem through netfslib which may invoke the cache, which may require cachefiles to check the xattr on the cache file and maybe set/remove it - which requires the sb_writers lock on the cache filesystem.

So if ->read_folio(), ->readahead() or ->writepages() can ever be called with mm->mmap_lock or vma->vm_lock held, netfslib may call down to cachefiles and ultimately, it should[*] then take the sb_writers lock on the backing filesystem to perform xattr manipulation.

[*] I say "should" because at the moment cachefiles calls vfs_set/removexattr
functions which *don't* take this lock (which is a bug). Is this an error on the part of vfs_set/removexattr()? Should they take this lock analogously with vfs_truncate() and vfs_iocb_iter_write()?

However, as it doesn't it manages to construct a locking chain via the mapping.invalidate_lock, the afs vnode->validate_lock and something in execve that I don't exactly follow.

I wonder if this is might be deadlockable by a multithreaded process (ie. so they share the mm locks) where one thread is writing to a cached file whilst another thread is trying to set/remove the xattr on that file.

Quoted 27 lines · David Howells

Quoted 9 lines · Jan Kara

Well, it seems like you are trying to get rid of the dependency sb_writers->mmap_sem. But there are other places where this dependency is created, in particular write(2) path is a place where it would be very difficult to get rid of it (you take sb_writers, then do all the work preparing the write and then you copy user data into page cache which may require mmap_sem).

...

Quoted 8 lines

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

I'm not sure you're correct about that. If you look at the lockdep splat:

Quoted 1 line

-> #2 (sb_writers#14){.+.+}-{0:0}:

The sb_writers lock is "personalised" to the filesystem type (the "#14" annotation) which is set here:

Quoted 8 lines

	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
		if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
					sb_writers_name[i],
					&type->s_writers_key[i]))  <----
			goto fail;
	}

in fs/super.c.

Right, forgot about that. Thanks for correction! So after pondering about this some more, this is actually a real problem and deadlockable. See below.

Quoted 4 lines

I think the problem is (1) that on one side, you've got, say, sys_setxattr() taking an sb_writers lock and then accessing a userspace buffer, which (a) may take mm->mmap_lock and vma->vm_lock and (b) may cause reading or writeback from the netfs-based filesystem via an mmapped xattr name buffer].

Yes, we agree on that. I was just pointing out that:

vfs_write()
file_start_write() -> grabs sb_writers
generic_file_write_iter()
generic_perform_write()
fault_in_iov_iter_readable()

is another path which enforces exactly the same lock ordering.

Quoted 9 lines

Then (2) on the other side, you have a read or a write to the network filesystem through netfslib which may invoke the cache, which may require cachefiles to check the xattr on the cache file and maybe set/remove it - which requires the sb_writers lock on the cache filesystem.

So if ->read_folio(), ->readahead() or ->writepages() can ever be called with mm->mmap_lock or vma->vm_lock held, netfslib may call down to cachefiles and ultimately, it should[*] then take the sb_writers lock on the backing filesystem to perform xattr manipulation.

Well, ->read_folio() under mm->mmap_lock is a standard thing to happen in a page fault. Now grabbing sb_writers (of any filesystem) in that path is problematic and can deadlock though:

page fault
grab mm->mmap_lock
filemap_fault()
if (unlikely(!folio_test_uptodate(folio))) {
filemap_read_folio() on fs A
now if you grab sb_writers on fs B:
freeze_super() on fs B write(2) on fs B
sb_start_write(fs B)
sb->s_writers.frozen = SB_FREEZE_WRITE; sb_wait_write(sb, SB_FREEZE_WRITE);
- waits for write
sb_start_write(fs B) - blocked behind freeze_super()
generic_perform_write()
fault_in_iov_iter_readable()
page fault
grab mm->mmap_lock
=> deadlock

Now this is not the deadlock your lockdep trace is showing but it is a similar one. Like:

filemap_invalidate() on fs A freeze_super() on fs B page fault on fs A write(2) on fs B
filemap_invalidate_lock() lock mm->mmap_lock sb_start_write(fs B)
filemap_fdatawrite_wbc() filemap_fault()
afs_writepages() filemap_invalidate_lock_shared()
cachefiles_issue_write() => blocks behind filemap_invalidate()
sb->s_writers.frozen = SB_FREEZE_WRITE;
sb_wait_write(sb, SB_FREEZE_WRITE);
=> blocks behind write(2)
sb_start_write() on fs B
=> blocks behind freeze_super()
generic_perform_write()
fault_in_iov_iter_readable()
page fault
grab mm->mmap_lock
=> deadlock

So I still maintain that grabbing sb_start_write() from quite deep within locking hierarchy (like from writepages when having pages locked, but even holding invalidate_lock is enough for the problems) is problematic and prone to deadlocks.

Quoted 10 lines

[*] I say "should" because at the moment cachefiles calls vfs_set/removexattr
functions which *don't* take this lock (which is a bug). Is this an error on the part of vfs_set/removexattr()? Should they take this lock analogously with vfs_truncate() and vfs_iocb_iter_write()?

However, as it doesn't it manages to construct a locking chain via the mapping.invalidate_lock, the afs vnode->validate_lock and something in execve that I don't exactly follow.

Quoted 3 lines

I wonder if this is might be deadlockable by a multithreaded process (ie. so they share the mm locks) where one thread is writing to a cached file whilst another thread is trying to set/remove the xattr on that file.

Yep, see above. Now the hard question is how to fix this because what you are doing seems to be inherent in how cachefiles fs is designed to work.

Quoted 71 lines · Jan Kara

Quoted 9 lines · David Howells

Then (2) on the other side, you have a read or a write to the network filesystem through netfslib which may invoke the cache, which may require cachefiles to check the xattr on the cache file and maybe set/remove it - which requires the sb_writers lock on the cache filesystem.

So if ->read_folio(), ->readahead() or ->writepages() can ever be called with mm->mmap_lock or vma->vm_lock held, netfslib may call down to cachefiles and ultimately, it should[*] then take the sb_writers lock on the backing filesystem to perform xattr manipulation.

Well, ->read_folio() under mm->mmap_lock is a standard thing to happen in a page fault. Now grabbing sb_writers (of any filesystem) in that path is problematic and can deadlock though:

page fault
grab mm->mmap_lock
filemap_fault()
if (unlikely(!folio_test_uptodate(folio))) {
filemap_read_folio() on fs A
now if you grab sb_writers on fs B:
freeze_super() on fs B write(2) on fs B
sb_start_write(fs B)
sb->s_writers.frozen = SB_FREEZE_WRITE; sb_wait_write(sb, SB_FREEZE_WRITE);
- waits for write
sb_start_write(fs B) - blocked behind freeze_super()
generic_perform_write()
fault_in_iov_iter_readable()
page fault
grab mm->mmap_lock
=> deadlock

Now this is not the deadlock your lockdep trace is showing but it is a similar one. Like:

filemap_invalidate() on fs A freeze_super() on fs B page fault on fs A write(2) on fs B
filemap_invalidate_lock() lock mm->mmap_lock sb_start_write(fs B)
filemap_fdatawrite_wbc() filemap_fault()
afs_writepages() filemap_invalidate_lock_shared()
cachefiles_issue_write() => blocks behind filemap_invalidate()
sb->s_writers.frozen = SB_FREEZE_WRITE;
sb_wait_write(sb, SB_FREEZE_WRITE);
=> blocks behind write(2)
sb_start_write() on fs B
=> blocks behind freeze_super()
generic_perform_write()
fault_in_iov_iter_readable()
page fault
grab mm->mmap_lock
=> deadlock

So I still maintain that grabbing sb_start_write() from quite deep within locking hierarchy (like from writepages when having pages locked, but even holding invalidate_lock is enough for the problems) is problematic and prone to deadlocks.

Quoted 13 lines

[*] I say "should" because at the moment cachefiles calls vfs_set/removexattr
functions which *don't* take this lock (which is a bug). Is this an error on the part of vfs_set/removexattr()? Should they take this lock analogously with vfs_truncate() and vfs_iocb_iter_write()?

However, as it doesn't it manages to construct a locking chain via the mapping.invalidate_lock, the afs vnode->validate_lock and something in execve that I don't exactly follow.

I wonder if this is might be deadlockable by a multithreaded process (ie. so they share the mm locks) where one thread is writing to a cached file whilst another thread is trying to set/remove the xattr on that file.

Quoted 2 lines

Yep, see above. Now the hard question is how to fix this because what you are doing seems to be inherent in how cachefiles fs is designed to work.

So one idea may be to create a private mount for cachefiles and then claim write access when that private mount is created and retaining that write access for the duration of cachefiles being run. See my draft patch below.

To fix this, either we'd have to keep the lower cache filesystem private to cachefiles (but I don't think that works with the usecases) or we have to somehow untangle this mmap_lock knot. This "page fault does quite some fs locking under mmap_lock" problem is not causing filesystems headaches for

the first time. I would *love* to be able to always drop mmap_lock in the page fault handler, fill the data into the page cache and then retry the fault (so that filemap_map_pages() would then handle the fault without filesystem involvement). It would make many things in filesystem locking simpler. As far as I'm checking there are now not that many places that could not handle dropping of mmap_lock during fault (traditionally the problem is with get_user_pages() / pin_user_pages() users). So maybe this dream would be feasible after all.

Quoted 4 lines · Jan Kara

To fix this, either we'd have to keep the lower cache filesystem private to cachefiles (but I don't think that works with the usecases) or we have to somehow untangle this mmap_lock knot. This "page fault does quite some fs locking under mmap_lock" problem is not causing filesystems headaches for

Quoted 8 lines

the first time. I would *love* to be able to always drop mmap_lock in the page fault handler, fill the data into the page cache and then retry the fault (so that filemap_map_pages() would then handle the fault without filesystem involvement). It would make many things in filesystem locking simpler. As far as I'm checking there are now not that many places that could not handle dropping of mmap_lock during fault (traditionally the problem is with get_user_pages() / pin_user_pages() users). So maybe this dream would be feasible after all.

The traditional problem was the array of VMAs which was removed in commit b2cac248191b -- if we dropped the mmap_lock, any previous entries in that array would become invalid. Now that array is gone, do we have any remaining dependencies on the VMAs remaining valid?

Quoted 49 lines · David Howells

======================================================
WARNING: possible circular locking dependency detected
6.10.0-build2+ #956 Not tainted
------------------------------------------------------
fsstress/6050 is trying to acquire lock:
ffff888138fd82f0 (mapping.invalidate_lock#3){++++}-{3:3}, at: filemap_fault+0x26e/0x8b0

but task is already holding lock:
ffff888113f26d18 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x165/0x250

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #4 (&vma->vm_lock->lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        down_write+0x3b/0x50
        vma_start_write+0x6b/0xa0
        vma_link+0xcc/0x140
        insert_vm_struct+0xb7/0xf0
        alloc_bprm+0x2c1/0x390
        kernel_execve+0x65/0x1a0
        call_usermodehelper_exec_async+0x14d/0x190
        ret_from_fork+0x24/0x40
        ret_from_fork_asm+0x1a/0x30

-> #3 (&mm->mmap_lock){++++}-{3:3}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        __might_fault+0x7c/0xb0
        strncpy_from_user+0x25/0x160
        removexattr+0x7f/0x100
        __do_sys_fremovexattr+0x7e/0xb0
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

-> #2 (sb_writers#14){.+.+}-{0:0}:

        __lock_acquire+0xaf0/0xd80
        lock_acquire.part.0+0x103/0x280
        percpu_down_read+0x3c/0x90
        vfs_iocb_iter_write+0xe9/0x1d0
        __cachefiles_write+0x367/0x430
        cachefiles_issue_write+0x299/0x2f0
        netfs_advance_write+0x117/0x140
        netfs_write_folio.isra.0+0x5ca/0x6e0
        netfs_writepages+0x230/0x2f0
        afs_writepages+0x4d/0x70
        do_writepages+0x1e8/0x3e0

Quoted 8 lines

        filemap_fdatawrite_wbc+0x84/0xa0
        __filemap_fdatawrite_range+0xa8/0xf0
        file_write_and_wait_range+0x59/0x90
        afs_release+0x10f/0x270
        __fput+0x25f/0x3d0
        __do_sys_close+0x43/0x70
        do_syscall_64+0x9f/0x100
        entry_SYSCALL_64_after_hwframe+0x76/0x7e

This is the problematic step - from quite deep in the locking chain holding invalidate_lock and having PG_Writeback set you suddently jump to very outer locking context grabbing sb_writers. Now AFAICT this is not a real deadlock problem because the locks are actually on different filesystems, just lockdep isn't able to see this. So I don't think you will get rid of these lockdep splats unless you somehow manage to convey to lockdep that there's the "upper" fs (AFS in this case) and the "lower" fs (the one behind cachefiles) and their locks are different.

This is to prevent the deadlocks on upper->lower->upper and lower->upper->lower filesystem recursion via GFP_KERNEL memory allocation and reclaim recursing between the two filesystems. This is especially relevant for filesystems with ->writepage methods that can be called from direct reclaim. Hence allocations in this path need to be at least NOFS to prevent recursion back into the upper filesystem from writeback into the lower filesystem.

fs/xattr.c +1 -0

@@ -954,15 +952,23 @@ SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname,
 SYSCALL_DEFINE2(fremovexattr, int, fd, const char __user *, name)
 {
 	struct fd f = fdget(fd);
+	char kname[XATTR_NAME_MAX + 1];
 	int error = -EBADF;

fs/xattr.c SYSCALL_DEFINE2(lremovexattr, const char __user *, pathname, · line 966

Quoted 5 lines

fs/xattr.c +5 -0

+	error = strncpy_from_user(kname, name, sizeof(kname));
+	if (error == 0 || error == sizeof(kname))
+		error = -ERANGE;
+	if (error < 0)
+		return error;

Missing fdput() here.