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, 24 May 2023 12:51:48 +0200
From:   Jan Kara <jack@suse.cz>
To:     David Laight <David.Laight@ACULAB.COM>
Cc:     'Jan Kara' <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
        "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        "Darrick J. Wong" <djwong@kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH] ext4: Fix possible corruption when moving a directory
 with RENAME_EXCHANGE
Message-ID: <20230524105148.wgjj7ayrbeol6cdx@quack3>
References: <20230523131408.13470-1-jack@suse.cz>
 <48d1f20b2fc1418080c96a1736f6249b@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <48d1f20b2fc1418080c96a1736f6249b@AcuMS.aculab.com>
Precedence: bulk

On Tue 23-05-23 13:50:01, David Laight wrote:
> From: Jan Kara
> > Sent: 23 May 2023 14:14
> > 
> > Commit 0813299c586b ("ext4: Fix possible corruption when moving a
> > directory") forgot that handling of RENAME_EXCHANGE renames needs the
> > protection of inode lock when changing directory parents for moved
> > directories. Add proper locking for that case as well.
> > 
> > CC: stable@vger.kernel.org
> > Fixes: 0813299c586b ("ext4: Fix possible corruption when moving a directory")
> > Reported-by: "Darrick J. Wong" <djwong@kernel.org>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext4/namei.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index 45b579805c95..b91abea1c781 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> > @@ -4083,10 +4083,25 @@ static int ext4_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> >  	if (retval)
> >  		return retval;
> > 
> > +	/*
> > +	 * We need to protect against old.inode and new.inode directory getting
> > +	 * converted from inline directory format into a normal one. The lock
> > +	 * ordering does not matter here as old and new are guaranteed to be
> > +	 * incomparable in the directory hierarchy.
> > +	 */
> > +	if (S_ISDIR(old.inode->i_mode))
> > +		inode_lock(old.inode);
> > +	if (S_ISDIR(new.inode->i_mode))
> > +		inode_lock_nested(new.inode, I_MUTEX_NONDIR2);
> > +
> 
> What happens if there is another concurrent rename from new.inode
> to old.inode?
> That will try to acquire the locks in the other order.

That is not really possible because these two renames cannot happen in
parallel due to VFS locking - either old & new share parent which is locked
by VFS (so there cannot be another rename in that directory) or they have
different parents which are also locked by VFS (so again it is not possible
to race with another rename in these two dirs).

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