linux-ext4 - Re: [PATCH] ext4: Fix possible corruption when moving a directory with RENAME

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxgizNA9e3rXmktU-pqCzoxg-=n4u_PAHczo1bgquba5Og@mail.gmail.com>
Date:   Wed, 24 May 2023 16:11:13 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Jan Kara <jack@...e.cz>
Cc:     David Laight <David.Laight@...lab.com>, Ted Tso <tytso@....edu>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "Darrick J. Wong" <djwong@...nel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] ext4: Fix possible corruption when moving a directory
 with RENAME_EXCHANGE

On Wed, May 24, 2023 at 2:27 PM Jan Kara <jack@...e.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@...r.kernel.org
> > > Fixes: 0813299c586b ("ext4: Fix possible corruption when moving a directory")
> > > Reported-by: "Darrick J. Wong" <djwong@...nel.org>
> > > Signed-off-by: Jan Kara <jack@...e.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).

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 => 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.