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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240723123202.vu5tfzoblpib3nve@quack3>
Date: Tue, 23 Jul 2024 14:32:02 +0200
From: Jan Kara <jack@...e.cz>
To: Christian Brauner <brauner@...nel.org>
Cc: Jan Kara <jack@...e.cz>, David Howells <dhowells@...hat.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	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()

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@...e.com>
SUSE Labs, CR