Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 Jan 2023 13:37:35 +0100
From:   Jan Kara <jack@suse.cz>
To:     Al Viro <viro@ZenIV.linux.org.uk>
Cc:     linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        Ted Tso <tytso@mit.edu>, linux-xfs@vger.kernel.org
Subject: Locking issue with directory renames
Message-ID: <20230117123735.un7wbamlbdihninm@quack3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

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?

								Honza

[1] https://lore.kernel.org/all/000000000000261eb005f2191696@google.com

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