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.
Thanks,
Amir.
