Re: Locking issue with directory renames

[Jan Kara <jack@suse.cz>]

On Tue 17-01-23 16:57:55, Al Viro wrote:
> On Tue, Jan 17, 2023 at 01:37:35PM +0100, Jan Kara wrote:
> > Hello!
> > 
> > I've some across an interesting issue that was spotted by syzbot [1]. The
> > report is against UDF but AFAICS the problem exists for ext4 as well and
> > possibly other filesystems. The problem is the following: When we are
> > renaming directory 'dir' say rename("foo/dir", "bar/") we lock 'foo' and
> > 'bar' but 'dir' is unlocked because the locking done by vfs_rename() is
> > 
> >         if (!is_dir || (flags & RENAME_EXCHANGE))
> >                 lock_two_nondirectories(source, target);
> >         else if (target)
> >                 inode_lock(target);
> > 
> > However some filesystems (e.g. UDF but ext4 as well, I suspect XFS may be
> > hurt by this as well because it converts among multiple dir formats) need
> > to update parent pointer in 'dir' and nothing protects this update against
> > a race with someone else modifying 'dir'. Now this is mostly harmless
> > because the parent pointer (".." directory entry) is at the beginning of
> > the directory and stable however if for example the directory is converted
> > from packed "in-inode" format to "expanded" format as a result of
> > concurrent operation on 'dir', the filesystem gets corrupted (or crashes as
> > in case of UDF).
> > 
> > So we'd need to lock 'source' if it is a directory. Ideally this would
> > happen in VFS as otherwise I bet a lot of filesystems will get this wrong
> > so could vfs_rename() lock 'source' if it is a dir as well? Essentially
> > this would amount to calling lock_two_nondirectories(source, target)
> > unconditionally but that would become a serious misnomer ;). Al, any
> > thought?
> 
> FWIW, I suspect that majority of filesystems that do implement rename
> do not have that problem.  Moreover, on cross-directory rename we already
> have
> 	* tree topology stabilized
> 	* source guaranteed not to be an ancestor of target or either of
> the parents
> so the method instance should be free to lock the source if it needs to
> do so.

Yes, we can lock the source inode in ->rename() if we need it. The snag is
that if 'target' exists, it is already locked so when locking 'source' we
are possibly not following the VFS lock ordering of i_rwsem by inode
address (I don't think it can cause any real dealock but still it looks
suspicious). Also we'll have to lock with I_MUTEX_NONDIR2 lockdep class to
make lockdep happy but that's just a minor annoyance. Finally, we'll have
to check for RENAME_EXCHANGE because in that case, both source and target
will be already locked. Thus if we do the additional locking in the
filesystem, we will leak quite some details about rename locking into the
filesystem which seems undesirable to me.

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