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

Message-ID: <CAOQ4uxhEzxvgpJ=_a++xdGAptsywc4gLmnJXBA7ipFmM+qHR3g@mail.gmail.com>
Date: Mon, 18 Aug 2025 13:16:02 +0200
From: Amir Goldstein <amir73il@...il.com>
To: NeilBrown <neil@...wn.name>
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, 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.

Thanks,
Amir.