linux-kernel - Re: [syzbot] [overlayfs?] WARNING in shmem

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] [day] [month] [year] [list]

Message-id: <175555294028.2234665.14790599995742040769@noble.neil.brown.name>
Date: Tue, 19 Aug 2025 07:35:40 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "syzbot" <syzbot+ec9fab8b7f0386b98a17@...kaller.appspotmail.com>,
 linux-kernel@...r.kernel.org, linux-unionfs@...r.kernel.org,
 miklos@...redi.hu, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [overlayfs?] WARNING in shmem_unlink

On Mon, 18 Aug 2025, Amir Goldstein wrote:
> On Mon, Aug 18, 2025 at 2:34 AM NeilBrown <neil@...wn.name> wrote:
> >
> > On Mon, 18 Aug 2025, Amir Goldstein wrote:
> > > Neil,
> > >
> > > I will have a look tomorrow.
> > > If you have ideas I am open to hear them.
> > > The repro is mounting overlayfs all over each other in concurrent threads
> > > and one of the rmdir of "work" dir triggers this assertion
> >
> > My guess is that by dropping and retaking the lock, we open the
> > possibility of a race so that by the time vfs_unlink() is called the
> > dentry has already been unlinked.  In that case it would be unhashed.
> > So after retaking the lock we need to check d_unhashed() as well as
> > ->d_parent.
> >
> > So something like
> > --- a/fs/overlayfs/util.c
> > +++ b/fs/overlayfs/util.c
> > @@ -1552,7 +1552,8 @@ void ovl_copyattr(struct inode *inode)
> >  int ovl_parent_lock(struct dentry *parent, struct dentry *child)
> >  {
> >         inode_lock_nested(parent->d_inode, I_MUTEX_PARENT);
> > -       if (!child || child->d_parent == parent)
> > +       if (!child ||
> > +           (!d_unhashed(child) && child->d_parent == parent))
> >                 return 0;
> >
> >         inode_unlock(parent->d_inode);
> >
> >
> > NeilBrown
> >
> 
> Nice!
> I pushed this commit to ovl-fixes:
> 
> commit c56976d86e11afcd6b23633395a7f2e6e920e42d (HEAD -> ovl-fixes)
> Author: Amir Goldstein <amir73il@...il.com>
> Date:   Mon Aug 18 11:23:55 2025 +0200
> 
>     ovl: fix possible double unlink
> 
>     commit 9d23967b18c6 ("ovl: simplify an error path in
>     ovl_copy_up_workdir()") introduced the helper ovl_cleanup_unlocked(),
>     which is later used in several following patches to re-acquire the parent
>     inode lock and unlink a dentry that was earlier found using lookup.
>     This helper was eventually renamed to ovl_cleanup().
> 
>     The helper ovl_parent_lock() is used to re-acquire the parent inode lock.
>     After acquiring the parent inode lock, the helper verifies that the
>     dentry has not since been moved to another parent, but it failed to
>     verify that the dentry wasn't unlinked from the parent.
> 
>     This means that now every call to ovl_cleanup() could potentially
>     race with another thread, unlinking the dentry to be cleaned up
>     underneath overlayfs and trigger a vfs assertion.
> 
>     Reported-by: syzbot+ec9fab8b7f0386b98a17@...kaller.appspotmail.com
>     Tested-by: syzbot+ec9fab8b7f0386b98a17@...kaller.appspotmail.com
>     Fixes: 9d23967b18c6 ("ovl: simplify an error path in ovl_copy_up_workdir()")
>     Suggested-by: NeilBrown <neil@...wn.name>
>     Signed-off-by: Amir Goldstein <amir73il@...il.com>
> 
> Neil,
> 
> Please review my commit message.
> If you want me to assign you ownership please sign off on this commit message.

Looks good to me.  No changes needed.

NeilBrown


> 
> Thanks,
> Amir.
>