On Mon, Aug 18, 2025 at 11:35 PM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> 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.
We are having some problems with this fix colliding with a new ovl feature [1].
Let's try to test this revised fix:
#syz test: https://github.com/amir73il/linux ovl_casefold
Thanks,
Amir.
[1] https://lore.kernel.org/linux-unionfs/CAOQ4uxj551a7cvjpcYEyTLtsEXw9OxHtTc-VSm170J5pWtwoUQ@xxxxxxxxxxxxxx/
