Re: [syzbot] [overlayfs?] WARNING in shmem_unlink

[NeilBrown]

On Mon, 18 Aug 2025, Amir Goldstein wrote:
> On Mon, Aug 18, 2025 at 2:34 AM NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
>     Tested-by: syzbot+ec9fab8b7f0386b98a17@xxxxxxxxxxxxxxxxxxxxxxxxx
>     Fixes: 9d23967b18c6 ("ovl: simplify an error path in ovl_copy_up_workdir()")
>     Suggested-by: NeilBrown <neil@xxxxxxxxxx>
>     Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> 
> 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.
>