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:   Wed, 18 Jan 2023 19:41:41 +0100
From:   Jan Kara <jack@suse.cz>
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org,
        linux-ext4@vger.kernel.org, Ted Tso <tytso@mit.edu>,
        linux-xfs@vger.kernel.org
Subject: Re: Locking issue with directory renames
Message-ID: <20230118184141.pppaeg7wcj3ierae@quack3>
References: <20230117123735.un7wbamlbdihninm@quack3>
 <Y8bTk1CsH9AaAnLt@ZenIV>
 <20230118091036.qqscls22q6htxscf@quack3>
 <Y8gejpDqxV6Uo/xY@ZenIV>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8gejpDqxV6Uo/xY@ZenIV>
Precedence: bulk

On Wed 18-01-23 16:30:06, Al Viro wrote:
> On Wed, Jan 18, 2023 at 10:10:36AM +0100, Jan Kara wrote:
> 
> > 
> > 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.
> 
> Rules for inode locks are simple:
> 	* directories before non-directories
> 	* ancestors before descendents
> 	* for non-directories the ordering is by in-core inode address
> 
> So the instances that need that extra lock would do that when source is
> a directory and non RENAME_EXCHANGE is given.  Having the target already
> locked is irrelevant - if it exists, it's already checked to be a directory
> as well, and had it been a descendent of source, we would have already
> found that and failed with -ELOOP.
> 
> If A and B are both directories, there's no ordering between them unless
> one is an ancestor of another - such can be locked in any order.
> However, one of the following must be true:
> 	* C is locked and both A and B had been observed to be children of C
> after the lock on C had been acquired, or
> 	* ->s_vfs_rename_mutex is held for the filesystem containing both
> A and B.
> 
> Note that ->s_vfs_rename_mutex is there to stabilize the tree topology and
> make "is A an ancestor of B?" possible to check for more than "A is locked,
> B is a child of A, so A will remain its ancestor until unlocked"...

OK, fair enough. I'll fix things inside UDF and ext4.

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