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