Message-ID: <2147168.1721743066@warthog.procyon.org.uk>
Date: Tue, 23 Jul 2024 14:57:46 +0100
From: David Howells <dhowells@...hat.com>
To: Jan Kara <jack@...e.cz>
Cc: dhowells@...hat.com, Alexander Viro <viro@...iv.linux.org.uk>,
    Christian Brauner <brauner@...nel.org>,
    Jeff Layton <jlayton@...nel.org>, Gao Xiang <xiang@...nel.org>,
    Matthew Wilcox <willy@...radead.org>, netfs@...ts.linux.dev,
    linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
    linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfs: Fix potential circular locking through setxattr() and removexattr()

Jan Kara <jack@...e.cz> wrote:

> 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.

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

>  -> #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

Powered by blists - more mailing lists