Re: [syzbot] [reiserfs?] [squashfs?] BUG: Dentry still in use in unmount

Re: [syzbot] [reiserfs?] [squashfs?] BUG: Dentry still in use in unmount

[syzbot]

syzbot has bisected this issue to:

commit c63e56a4a6523fcb1358e1878607d77a40b534bb
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Wed Aug 16 09:42:18 2023 +0000

    ovl: do not open/llseek lower file with upper sb_writers held

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=13723c01e80000
start commit:   3bd7d7488169 Merge tag 'io_uring-6.7-2023-12-15' of git://..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=10f23c01e80000
console output: https://syzkaller.appspot.com/x/log.txt?x=17723c01e80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=53ec3da1d259132f
dashboard link: https://syzkaller.appspot.com/bug?extid=8608bb4553edb8c78f41
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17b8b6e1e80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16ec773ae80000

Reported-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com
Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[PATCH] ovl: fix BUG: Dentry still in use in unmount

[Edward Adam Davis]

workdir and destdir could be the same when copying up to indexdir.

Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held")
Reported-and-tested-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/overlayfs/copy_up.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 4382881b0709..ae5eb442025d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -731,10 +731,14 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		.rdev = c->stat.rdev,
 		.link = c->link
 	};
+	err = -EIO;
+	/* workdir and destdir could be the same when copying up to indexdir */
+	if (lock_rename(c->workdir, c->destdir) != NULL)
+		goto unlock;
 
 	err = ovl_prep_cu_creds(c->dentry, &cc);
 	if (err)
-		return err;
+		goto unlock;
 
 	ovl_start_write(c->dentry);
 	inode_lock(wdir);
@@ -743,8 +747,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_end_write(c->dentry);
 	ovl_revert_cu_creds(&cc);
 
+	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
-		return PTR_ERR(temp);
+		goto unlock;
 
 	/*
 	 * Copy up data first and then xattrs. Writing data after
@@ -760,10 +765,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	 * If temp was moved, abort without the cleanup.
 	 */
 	ovl_start_write(c->dentry);
-	if (lock_rename(c->workdir, c->destdir) != NULL ||
-	    temp->d_parent != c->workdir) {
+	if (temp->d_parent != c->workdir) {
 		err = -EIO;
-		goto unlock;
+		goto unlockcd;
 	} else if (err) {
 		goto cleanup;
 	}
@@ -801,16 +805,18 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	ovl_inode_update(inode, temp);
 	if (S_ISDIR(inode->i_mode))
 		ovl_set_flag(OVL_WHITEOUTS, inode);
+
+unlockcd:
+	ovl_end_write(c->dentry);
 unlock:
 	unlock_rename(c->workdir, c->destdir);
-	ovl_end_write(c->dentry);
 
 	return err;
 
 cleanup:
 	ovl_cleanup(ofs, wdir, temp);
 	dput(temp);
-	goto unlock;
+	goto unlockcd;
 }
 
 /* Copyup using O_TMPFILE which does not require cross dir locking */
-- 
2.43.0

Re: [syzbot] [reiserfs?] [squashfs?] BUG: Dentry still in use in unmount

[Amir Goldstein]

On Sun, Dec 17, 2023 at 1:19 AM syzbot
<syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com> wrote:
>
> syzbot has bisected this issue to:
>
> commit c63e56a4a6523fcb1358e1878607d77a40b534bb
> Author: Amir Goldstein <amir73il@gmail.com>
> Date:   Wed Aug 16 09:42:18 2023 +0000
>
>     ovl: do not open/llseek lower file with upper sb_writers held
>
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=13723c01e80000
> start commit:   3bd7d7488169 Merge tag 'io_uring-6.7-2023-12-15' of git://..
> git tree:       upstream
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=10f23c01e80000
> console output: https://syzkaller.appspot.com/x/log.txt?x=17723c01e80000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=53ec3da1d259132f
> dashboard link: https://syzkaller.appspot.com/bug?extid=8608bb4553edb8c78f41
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=17b8b6e1e80000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=16ec773ae80000
>
> Reported-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com
> Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held")
>

#syz test: https://github.com/amir73il/linux ovl-fixes

Re: [PATCH] ovl: fix BUG: Dentry still in use in unmount

[Amir Goldstein]

Hi Edward,

Thanks for the quick fix, but it is incorrect.

On Sun, Dec 17, 2023 at 10:11 AM Edward Adam Davis <eadavis@qq.com> wrote:
>
> workdir and destdir could be the same when copying up to indexdir.

This is not the reason for the bug, the reason is:

    syzbot exercised the forbidden practice of moving the workdir under
    lowerdir while overlayfs is mounted and tripped a dentry reference leak.

>
> Fixes: c63e56a4a652 ("ovl: do not open/llseek lower file with upper sb_writers held")
> Reported-and-tested-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  fs/overlayfs/copy_up.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 4382881b0709..ae5eb442025d 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -731,10 +731,14 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>                 .rdev = c->stat.rdev,
>                 .link = c->link
>         };
> +       err = -EIO;
> +       /* workdir and destdir could be the same when copying up to indexdir */
> +       if (lock_rename(c->workdir, c->destdir) != NULL)
> +               goto unlock;

You can't do that. See comment below ovl_copy_up_data().

>
>         err = ovl_prep_cu_creds(c->dentry, &cc);
>         if (err)
> -               return err;
> +               goto unlock;
>
>         ovl_start_write(c->dentry);
>         inode_lock(wdir);
> @@ -743,8 +747,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         ovl_end_write(c->dentry);
>         ovl_revert_cu_creds(&cc);
>
> +       err = PTR_ERR(temp);
>         if (IS_ERR(temp))
> -               return PTR_ERR(temp);
> +               goto unlock;
>
>         /*
>          * Copy up data first and then xattrs. Writing data after
> @@ -760,10 +765,9 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>          * If temp was moved, abort without the cleanup.
>          */
>         ovl_start_write(c->dentry);
> -       if (lock_rename(c->workdir, c->destdir) != NULL ||
> -           temp->d_parent != c->workdir) {
> +       if (temp->d_parent != c->workdir) {

                  dput(temp);

here is all that should be needed to fix the leak.


>                 err = -EIO;
> -               goto unlock;
> +               goto unlockcd;
>         } else if (err) {
>                 goto cleanup;
>         }

See my suggested fix at https://github.com/amir73il/linux/commits/ovl-fixes

Al,

Heads up.
This fix will have a minor conflict with a8b0026847b8 ("rename(): avoid
a deadlock in the case of parents having no common ancestor")
on your work.rename branch.

I plan to push my fix to linux-next soon, but I see that work.rename
is not in linux-next yet.

Thanks,
Amir.

Re: [syzbot] [reiserfs?] [squashfs?] BUG: Dentry still in use in unmount

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8608bb4553edb8c78f41@syzkaller.appspotmail.com

Tested on:

commit:         79f938ea ovl: fix dentry reference leak after changes ..
git tree:       https://github.com/amir73il/linux ovl-fixes
console output: https://syzkaller.appspot.com/x/log.txt?x=171ae01ae80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=52c9552def2a0fdd
dashboard link: https://syzkaller.appspot.com/bug?extid=8608bb4553edb8c78f41
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.