Re: [PATCH] vfs: Fix potential circular locking through setxattr() and removexattr()

[Jan Kara <jack@suse.cz>]

On Tue 23-07-24 13:11:51, Christian Brauner wrote:
> On Tue, Jul 23, 2024 at 12:45:33PM GMT, Jan Kara wrote:
> > On Tue 23-07-24 09:59:54, David Howells wrote:
> > > 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.
> > 
> > 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.

Oh, absolutely. I think the change itself makes sense, just it will not fix
what David hopes to fix :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR