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;
MIME-Version: 1.0
References: <20230523131408.13470-1-jack@suse.cz> <48d1f20b2fc1418080c96a1736f6249b@AcuMS.aculab.com>
 <20230524105148.wgjj7ayrbeol6cdx@quack3>
In-Reply-To: <20230524105148.wgjj7ayrbeol6cdx@quack3>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 24 May 2023 16:11:13 +0300
Message-ID: <CAOQ4uxgizNA9e3rXmktU-pqCzoxg-=n4u_PAHczo1bgquba5Og@mail.gmail.com>
Subject: Re: [PATCH] ext4: Fix possible corruption when moving a directory
 with RENAME_EXCHANGE
To:     Jan Kara <jack@suse.cz>
Cc:     David Laight <David.Laight@aculab.com>, 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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 24, 2023 at 2:27=E2=80=AFPM Jan Kara <jack@suse.cz> wrote:
>
> 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 dir=
ectory")
> > > 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 *ol=
d_dir, struct dentry *old_dentry,
> > >     if (retval)
> > >             return retval;
> > >
> > > +   /*
> > > +    * We need to protect against old.inode and new.inode directory g=
etting
> > > +    * 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 lock=
ed
> 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 possib=
le
> to race with another rename in these two dirs).

Unless D1/A ; D1/B are hardlinks of D2/B ; D2/A respectively
and exchange (D1/A, D1/B) is racing with exchange (D2/B, D2/A)

There is a simple solution of course, same as xfs_lock_two_inodes()

Another possible deadlock (I think) is if D/A ; D/B are subdirs that
are exchanged and after taking inode_lock of D and A, rename comes
in D/B/foo =3D> D/A/foo and lock_rename() tries to
lock_two_directories(B, A).

So it seems that both lock_two_directories() and to be helper
lock_two_inodes() need to order the two inodes by address?

Thanks,
Amir.