[RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
no need to mess with ->s_umount.

Objections?
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 7121d8c7a318..a3634e7f2304 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
  * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
  * work with different options work we need to keep backward compatibility.
  */
-static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
+static int btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
 	int ret = 0;
 
-	if (fc->sb_flags & SB_RDONLY)
-		return ret;
-
-	down_write(&mnt->mnt_sb->s_umount);
-	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
+	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
 		ret = btrfs_reconfigure(fc);
-	up_write(&mnt->mnt_sb->s_umount);
+
 	return ret;
 }
 
@@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
 	security_free_mnt_opts(&fc->security);
 	fc->security = NULL;
 
-	mnt = fc_mount(dup_fc);
-	if (IS_ERR(mnt)) {
-		put_fs_context(dup_fc);
-		return PTR_ERR(mnt);
+	ret = vfs_get_tree(dup_fc);
+	if (!ret) {
+		ret = btrfs_reconfigure_for_mount(dup_fc);
+		up_write(&fc->root->d_sb->s_umount);
 	}
-	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
+	if (!ret)
+		mnt = vfs_create_mount(fc);
+	else
+		mnt = ERR_PTR(ret);
 	put_fs_context(dup_fc);
-	if (ret) {
-		mntput(mnt);
-		return ret;
-	}
+	if (IS_ERR(mnt))
+		return PTR_ERR(mnt);
 
 	/*
 	 * This free's ->subvol_name, because if it isn't set we have to

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[David Sterba]

On Mon, May 05, 2025 at 04:03:45AM +0100, Al Viro wrote:
> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> no need to mess with ->s_umount.
> 
> Objections?
>     
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7121d8c7a318..a3634e7f2304 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
>   * work with different options work we need to keep backward compatibility.
>   */
> -static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
> +static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>  {
>  	int ret = 0;
>  
> -	if (fc->sb_flags & SB_RDONLY)
> -		return ret;
> -
> -	down_write(&mnt->mnt_sb->s_umount);
> -	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> +	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
>  		ret = btrfs_reconfigure(fc);
> -	up_write(&mnt->mnt_sb->s_umount);
> +
>  	return ret;
>  }
>  
> @@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
>  	security_free_mnt_opts(&fc->security);
>  	fc->security = NULL;
>  
> -	mnt = fc_mount(dup_fc);
> -	if (IS_ERR(mnt)) {
> -		put_fs_context(dup_fc);
> -		return PTR_ERR(mnt);
> +	ret = vfs_get_tree(dup_fc);

So this open codes fc_mount(), which is vfs_get_tree() + vfs_create_mount(),
the only difference I see in the new code is that
btrfs_reconfigure_for_mount() dropped the SB_RDONLY check.

Why the check is there is explained in the lengthy comment above
btrfs_reconfigure_for_mount(), so it should stay. If it can be removed
then it should be a separate patch from the cleanup.

> +	if (!ret) {
> +		ret = btrfs_reconfigure_for_mount(dup_fc);
> +		up_write(&fc->root->d_sb->s_umount);
>  	}
> -	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
> +	if (!ret)
> +		mnt = vfs_create_mount(fc);
> +	else
> +		mnt = ERR_PTR(ret);
>  	put_fs_context(dup_fc);
> -	if (ret) {
> -		mntput(mnt);
> -		return ret;
> -	}
> +	if (IS_ERR(mnt))
> +		return PTR_ERR(mnt);
>  
>  	/*
>  	 * This free's ->subvol_name, because if it isn't set we have to

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Mon, May 05, 2025 at 07:58:07PM +0200, David Sterba wrote:

> > -	if (fc->sb_flags & SB_RDONLY)
> > -		return ret;
> > -
> > -	down_write(&mnt->mnt_sb->s_umount);
> > -	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> > +	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
> >  		ret = btrfs_reconfigure(fc);
> > -	up_write(&mnt->mnt_sb->s_umount);

> So this open codes fc_mount(), which is vfs_get_tree() + vfs_create_mount(),
> the only difference I see in the new code is that
> btrfs_reconfigure_for_mount() dropped the SB_RDONLY check.
> 
> Why the check is there is explained in the lengthy comment above
> btrfs_reconfigure_for_mount(), so it should stay. If it can be removed
> then it should be a separate patch from the cleanup.

What do you mean, dropped?  It's still right there - the current
variant checks it *twice*, once before grabbing ->s_umount, then
after it's been grabbed.  Checking it before down_write() makes sense
if we are called after ->s_umount had been dropped (by fc_mount()).
I'm not sure why you recheck it after down_write(), since it's not
going to change, but you do recheck it.  In this variant we don't need
to bother grabbing the rwsem, since that thing is called while ->s_umount
is still held...

I can turn that into
	if (fc->sb_flags & SB_RDONLY)
		return ret;
	if (fc->root->d_sb->s_flags & SB_RDONLY)
		ret = btrfs_reconfigure(fc);
	return ret;
but I don't see how it's better than the variant posted; up to you, of course...

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 9130 bytes --]

Hi,

On 2025-05-05 04:03:45 +0100, Al Viro wrote:
> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> no need to mess with ->s_umount.
> 
> Objections?
>     

I hit an oops on today's next-20250506 which seems to point here, and
reverting makes it go away.

Let me know if there's anything else you need.

Regards,
Klara Modin

BUG: kernel NULL pointer dereference, address: 0000000000000068
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP NOPTI
CPU: 0 UID: 0 PID: 154 Comm: mount Not tainted 6.15.0-rc5-next-20250506 #495 PREEMPTLAZY
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-stable202408-prebuilt.qemu.org 08/13/2024
RIP: 0010:btrfs_get_tree (fs/btrfs/super.c:2065 fs/btrfs/super.c:2106) 
Code: 24 e8 40 3d 96 00 8b 1c 24 e9 e9 fd ff ff f6 83 a8 00 00 00 01 75 0e 48 8b 43 60 48 8b 40 68 f6 40 50 01 75 48 49 8b 44 24 60 <48> 8b 78 68 48 83 c7 70 e8 2f c2 b3 ff 4c 89 e7 e8 d7 87 e5 ff 49
All code
========
   0:	24 e8                	and    $0xe8,%al
   2:	40 3d 96 00 8b 1c    	rex cmp $0x1c8b0096,%eax
   8:	24 e9                	and    $0xe9,%al
   a:	e9 fd ff ff f6       	jmp    0xfffffffff700000c
   f:	83 a8 00 00 00 01 75 	subl   $0x75,0x1000000(%rax)
  16:	0e                   	(bad)
  17:	48 8b 43 60          	mov    0x60(%rbx),%rax
  1b:	48 8b 40 68          	mov    0x68(%rax),%rax
  1f:	f6 40 50 01          	testb  $0x1,0x50(%rax)
  23:	75 48                	jne    0x6d
  25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
  2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
  2e:	48 83 c7 70          	add    $0x70,%rdi
  32:	e8 2f c2 b3 ff       	call   0xffffffffffb3c266
  37:	4c 89 e7             	mov    %r12,%rdi
  3a:	e8 d7 87 e5 ff       	call   0xffffffffffe58816
  3f:	49                   	rex.WB

Code starting with the faulting instruction
===========================================
   0:	48 8b 78 68          	mov    0x68(%rax),%rdi
   4:	48 83 c7 70          	add    $0x70,%rdi
   8:	e8 2f c2 b3 ff       	call   0xffffffffffb3c23c
   d:	4c 89 e7             	mov    %r12,%rdi
  10:	e8 d7 87 e5 ff       	call   0xffffffffffe587ec
  15:	49                   	rex.WB
RSP: 0018:ffffc15200583df8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff9f49450f7e40 RCX: 0000000000000000
RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: ffffffffaf773b50
RBP: 0000000000000000 R08: ffff9f4941c5dca8 R09: ffff9f4942054d00
R10: ffffffffad2c3ce2 R11: ffff9f494482e000 R12: ffff9f4945224a80
R13: 0000000000000000 R14: ffff9f4944957f40 R15: 00000000ffffffff
FS:  00007f9b03861b80(0000) GS:ffff9f49ccd7b000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000068 CR3: 0000000005385000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
? vfs_parse_fs_string (fs/fs_context.c:191) 
vfs_get_tree (fs/super.c:1810) 
path_mount (fs/namespace.c:3882 fs/namespace.c:4208) 
__x64_sys_mount (fs/namespace.c:4222 fs/namespace.c:4432 fs/namespace.c:4409 fs/namespace.c:4409) 
do_syscall_64 (arch/x86/entry/syscall_64.c:63 (discriminator 1) arch/x86/entry/syscall_64.c:94 (discriminator 1)) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
RIP: 0033:0x7f9b0397ffae
[ 0.869107] Code: 48 8b 0d 4d 5e 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1a 5e 0d 00 f7 d8 64 89 01 48
All code
========
   0:	48 8b 0d 4d 5e 0d 00 	mov    0xd5e4d(%rip),%rcx        # 0xd5e54
   7:	f7 d8                	neg    %eax
   9:	64 89 01             	mov    %eax,%fs:(%rcx)
   c:	48 83 c8 ff          	or     $0xffffffffffffffff,%rax
  10:	c3                   	ret
  11:	66 2e 0f 1f 84 00 00 	cs nopw 0x0(%rax,%rax,1)
  18:	00 00 00 
  1b:	90                   	nop
  1c:	f3 0f 1e fa          	endbr64
  20:	49 89 ca             	mov    %rcx,%r10
  23:	b8 a5 00 00 00       	mov    $0xa5,%eax
  28:	0f 05                	syscall
  2a:*	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax		<-- trapping instruction
  30:	73 01                	jae    0x33
  32:	c3                   	ret
  33:	48 8b 0d 1a 5e 0d 00 	mov    0xd5e1a(%rip),%rcx        # 0xd5e54
  3a:	f7 d8                	neg    %eax
  3c:	64 89 01             	mov    %eax,%fs:(%rcx)
  3f:	48                   	rex.W

Code starting with the faulting instruction
===========================================
   0:	48 3d 01 f0 ff ff    	cmp    $0xfffffffffffff001,%rax
   6:	73 01                	jae    0x9
   8:	c3                   	ret
   9:	48 8b 0d 1a 5e 0d 00 	mov    0xd5e1a(%rip),%rcx        # 0xd5e2a
  10:	f7 d8                	neg    %eax
  12:	64 89 01             	mov    %eax,%fs:(%rcx)
  15:	48                   	rex.W
RSP: 002b:00007ffdca32ba68 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000000008000 RCX: 00007f9b0397ffae
RDX: 00005595a592a2e0 RSI: 00007ffdca32cf7b RDI: 00007ffdca32cf6e
RBP: 00007ffdca32cf6e R08: 00005595a59292e0 R09: 0000000000000002
R10: 0000000000008000 R11: 0000000000000202 R12: 00007ffdca32cf7b
R13: 00005595a592a2e0 R14: 0000000000008000 R15: 00005595a59292e0
 </TASK>
Modules linked in:
CR2: 0000000000000068
---[ end trace 0000000000000000 ]---
RIP: 0010:btrfs_get_tree (fs/btrfs/super.c:2065 fs/btrfs/super.c:2106) 
Code: 24 e8 40 3d 96 00 8b 1c 24 e9 e9 fd ff ff f6 83 a8 00 00 00 01 75 0e 48 8b 43 60 48 8b 40 68 f6 40 50 01 75 48 49 8b 44 24 60 <48> 8b 78 68 48 83 c7 70 e8 2f c2 b3 ff 4c 89 e7 e8 d7 87 e5 ff 49
All code
========
   0:	24 e8                	and    $0xe8,%al
   2:	40 3d 96 00 8b 1c    	rex cmp $0x1c8b0096,%eax
   8:	24 e9                	and    $0xe9,%al
   a:	e9 fd ff ff f6       	jmp    0xfffffffff700000c
   f:	83 a8 00 00 00 01 75 	subl   $0x75,0x1000000(%rax)
  16:	0e                   	(bad)
  17:	48 8b 43 60          	mov    0x60(%rbx),%rax
  1b:	48 8b 40 68          	mov    0x68(%rax),%rax
  1f:	f6 40 50 01          	testb  $0x1,0x50(%rax)
  23:	75 48                	jne    0x6d
  25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
  2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
  2e:	48 83 c7 70          	add    $0x70,%rdi
  32:	e8 2f c2 b3 ff       	call   0xffffffffffb3c266
  37:	4c 89 e7             	mov    %r12,%rdi
  3a:	e8 d7 87 e5 ff       	call   0xffffffffffe58816
  3f:	49                   	rex.WB

Code starting with the faulting instruction
===========================================
   0:	48 8b 78 68          	mov    0x68(%rax),%rdi
   4:	48 83 c7 70          	add    $0x70,%rdi
   8:	e8 2f c2 b3 ff       	call   0xffffffffffb3c23c
   d:	4c 89 e7             	mov    %r12,%rdi
  10:	e8 d7 87 e5 ff       	call   0xffffffffffe587ec
  15:	49                   	rex.WB
RSP: 0018:ffffc15200583df8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff9f49450f7e40 RCX: 0000000000000000
RDX: 7fffffffffffffff RSI: 0000000000000000 RDI: ffffffffaf773b50
RBP: 0000000000000000 R08: ffff9f4941c5dca8 R09: ffff9f4942054d00
R10: ffffffffad2c3ce2 R11: ffff9f494482e000 R12: ffff9f4945224a80
R13: 0000000000000000 R14: ffff9f4944957f40 R15: 00000000ffffffff
FS:  00007f9b03861b80(0000) GS:ffff9f49ccd7b000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000068 CR3: 0000000005385000 CR4: 0000000000350ef0
note: mount[154] exited with irqs disabled


> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7121d8c7a318..a3634e7f2304 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
>   * work with different options work we need to keep backward compatibility.
>   */
> -static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
> +static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>  {
>  	int ret = 0;
>  
> -	if (fc->sb_flags & SB_RDONLY)
> -		return ret;
> -
> -	down_write(&mnt->mnt_sb->s_umount);
> -	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> +	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
>  		ret = btrfs_reconfigure(fc);
> -	up_write(&mnt->mnt_sb->s_umount);
> +
>  	return ret;
>  }
>  
> @@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
>  	security_free_mnt_opts(&fc->security);
>  	fc->security = NULL;
>  
> -	mnt = fc_mount(dup_fc);
> -	if (IS_ERR(mnt)) {
> -		put_fs_context(dup_fc);
> -		return PTR_ERR(mnt);
> +	ret = vfs_get_tree(dup_fc);
> +	if (!ret) {
> +		ret = btrfs_reconfigure_for_mount(dup_fc);
> +		up_write(&fc->root->d_sb->s_umount);
>  	}
> -	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
> +	if (!ret)
> +		mnt = vfs_create_mount(fc);
> +	else
> +		mnt = ERR_PTR(ret);
>  	put_fs_context(dup_fc);
> -	if (ret) {
> -		mntput(mnt);
> -		return ret;
> -	}
> +	if (IS_ERR(mnt))
> +		return PTR_ERR(mnt);
>  
>  	/*
>  	 * This free's ->subvol_name, because if it isn't set we have to

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45130 bytes --]

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:
> Hi,
> 
> On 2025-05-05 04:03:45 +0100, Al Viro wrote:
> > it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> > no need to mess with ->s_umount.
> > 
> > Objections?
> >     
> 
> I hit an oops on today's next-20250506 which seems to point here, and
> reverting makes it go away.
> 
> Let me know if there's anything else you need.

.config and toolchain information would be useful...

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 17:43:10 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:
> > Hi,
> > 
> > On 2025-05-05 04:03:45 +0100, Al Viro wrote:
> > > it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> > > no need to mess with ->s_umount.
> > > 
> > > Objections?
> > >     
> > 
> > I hit an oops on today's next-20250506 which seems to point here, and
> > reverting makes it go away.
> > 
> > Let me know if there's anything else you need.
> 
> .config and toolchain information would be useful...

The .config should already be attached (config.gz), though it's possible
there's an issue with my email setup. It seems to be on lore at least.

I'm using gcc 15.1 and binutils 2.44.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:

>   25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
	rax = fc->root
>   2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
	rdi = rax->d_sb, hitting rax == 0

> > -	mnt = fc_mount(dup_fc);
> > -	if (IS_ERR(mnt)) {
> > -		put_fs_context(dup_fc);
> > -		return PTR_ERR(mnt);
> > +	ret = vfs_get_tree(dup_fc);
> > +	if (!ret) {
> > +		ret = btrfs_reconfigure_for_mount(dup_fc);
> > +		up_write(&fc->root->d_sb->s_umount);

... here.  D'oh...  Should be dup_fc, obviously - fc->root hadn't been
set yet.  Make that line
		up_write(&dup_fc->root->d_sb->s_umount);
and see if it helps.  Sorry about the braino...

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 18:25:39 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:
> 
> >   25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
> 	rax = fc->root
> >   2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
> 	rdi = rax->d_sb, hitting rax == 0
> 
> > > -	mnt = fc_mount(dup_fc);
> > > -	if (IS_ERR(mnt)) {
> > > -		put_fs_context(dup_fc);
> > > -		return PTR_ERR(mnt);
> > > +	ret = vfs_get_tree(dup_fc);
> > > +	if (!ret) {
> > > +		ret = btrfs_reconfigure_for_mount(dup_fc);
> > > +		up_write(&fc->root->d_sb->s_umount);
> 
> ... here.  D'oh...  Should be dup_fc, obviously - fc->root hadn't been
> set yet.  Make that line
> 		up_write(&dup_fc->root->d_sb->s_umount);
> and see if it helps.  Sorry about the braino...

Thanks, that fixes the oops for me.

Though now I hit another issue which I don't know if it's related or
not. I'm using an overlay mount with squashfs as lower and btrfs as
upper. The mount fails with invalid argument and I see this in the log:

overlayfs: failed to clone upperpath

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 07:47:29PM +0200, Klara Modin wrote:
> On 2025-05-06 18:25:39 +0100, Al Viro wrote:
> > On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:
> > 
> > >   25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
> > 	rax = fc->root
> > >   2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
> > 	rdi = rax->d_sb, hitting rax == 0
> > 
> > > > -	mnt = fc_mount(dup_fc);
> > > > -	if (IS_ERR(mnt)) {
> > > > -		put_fs_context(dup_fc);
> > > > -		return PTR_ERR(mnt);
> > > > +	ret = vfs_get_tree(dup_fc);
> > > > +	if (!ret) {
> > > > +		ret = btrfs_reconfigure_for_mount(dup_fc);
> > > > +		up_write(&fc->root->d_sb->s_umount);
> > 
> > ... here.  D'oh...  Should be dup_fc, obviously - fc->root hadn't been
> > set yet.  Make that line
> > 		up_write(&dup_fc->root->d_sb->s_umount);
> > and see if it helps.  Sorry about the braino...
> 
> Thanks, that fixes the oops for me.
> 
> Though now I hit another issue which I don't know if it's related or
> not. I'm using an overlay mount with squashfs as lower and btrfs as
> upper. The mount fails with invalid argument and I see this in the log:
> 
> overlayfs: failed to clone upperpath

Seeing that you already have a kernel with that thing reverted, could
you check if the problem exists there?

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 18:51:04 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 07:47:29PM +0200, Klara Modin wrote:
> > On 2025-05-06 18:25:39 +0100, Al Viro wrote:
> > > On Tue, May 06, 2025 at 03:36:03PM +0200, Klara Modin wrote:
> > > 
> > > >   25:	49 8b 44 24 60       	mov    0x60(%r12),%rax
> > > 	rax = fc->root
> > > >   2a:*	48 8b 78 68          	mov    0x68(%rax),%rdi		<-- trapping instruction
> > > 	rdi = rax->d_sb, hitting rax == 0
> > > 
> > > > > -	mnt = fc_mount(dup_fc);
> > > > > -	if (IS_ERR(mnt)) {
> > > > > -		put_fs_context(dup_fc);
> > > > > -		return PTR_ERR(mnt);
> > > > > +	ret = vfs_get_tree(dup_fc);
> > > > > +	if (!ret) {
> > > > > +		ret = btrfs_reconfigure_for_mount(dup_fc);
> > > > > +		up_write(&fc->root->d_sb->s_umount);
> > > 
> > > ... here.  D'oh...  Should be dup_fc, obviously - fc->root hadn't been
> > > set yet.  Make that line
> > > 		up_write(&dup_fc->root->d_sb->s_umount);
> > > and see if it helps.  Sorry about the braino...
> > 
> > Thanks, that fixes the oops for me.
> > 
> > Though now I hit another issue which I don't know if it's related or
> > not. I'm using an overlay mount with squashfs as lower and btrfs as
> > upper. The mount fails with invalid argument and I see this in the log:
> > 
> > overlayfs: failed to clone upperpath
> 
> Seeing that you already have a kernel with that thing reverted, could
> you check if the problem exists there?

Yeah, it works fine with the revert instead.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 07:54:32PM +0200, Klara Modin wrote:

> > > Though now I hit another issue which I don't know if it's related or
> > > not. I'm using an overlay mount with squashfs as lower and btrfs as
> > > upper. The mount fails with invalid argument and I see this in the log:
> > > 
> > > overlayfs: failed to clone upperpath
> > 
> > Seeing that you already have a kernel with that thing reverted, could
> > you check if the problem exists there?
> 
> Yeah, it works fine with the revert instead.

Interesting...  That message means that you've got clone_private_mount()
returning an error; the thing is, mount passed to it has come from
pathname lookup - it is *not* the mount created by that fc_mount() of
vfs_create_mount() in the modified code.  That one gets passed to
mount_subvol() and consumed there (by mount_subtree()).  All that is returned
is root dentry; the mount passed to clone_private_mount() is created
from scratch using dentry left by btrfs_get_tree_subvol() in its fc->root -
see
        dentry = mount_subvol(ctx->subvol_name, ctx->subvol_objectid, mnt);
        ctx->subvol_name = NULL;
        if (IS_ERR(dentry))
                return PTR_ERR(dentry);

        fc->root = dentry;
        return 0;
in the end of btrfs_get_tree_subvol().

What's more, on the overlayfs side we managed to get to
        upper_mnt = clone_private_mount(upperpath);
        err = PTR_ERR(upper_mnt);
        if (IS_ERR(upper_mnt)) {
                pr_err("failed to clone upperpath\n");
                goto out;
so the upper path had been resolved...

OK, let's try to see what clone_private_mount() is unhappy about...
Could you try the following on top of -next + braino fix and see
what shows up?  Another interesting thing, assuming you can get
to shell after overlayfs mount failure, would be /proc/self/mountinfo
contents and stat(1) output for upper path of your overlayfs mount...

diff --git a/fs/namespace.c b/fs/namespace.c
index eb990e9a668a..f7ce53f437dd 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2480,31 +2480,45 @@ struct vfsmount *clone_private_mount(const struct path *path)
 
 	guard(rwsem_read)(&namespace_sem);
 
-	if (IS_MNT_UNBINDABLE(old_mnt))
+	if (IS_MNT_UNBINDABLE(old_mnt)) {
+		pr_err("unbindable");
 		return ERR_PTR(-EINVAL);
+	}
 
 	if (mnt_has_parent(old_mnt)) {
-		if (!check_mnt(old_mnt))
+		if (!check_mnt(old_mnt)) {
+			pr_err("mounted, but not in our namespace");
 			return ERR_PTR(-EINVAL);
+		}
 	} else {
-		if (!is_mounted(&old_mnt->mnt))
+		if (!is_mounted(&old_mnt->mnt)) {
+			pr_err("not mounted");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure this isn't something purely kernel internal. */
-		if (!is_anon_ns(old_mnt->mnt_ns))
+		if (!is_anon_ns(old_mnt->mnt_ns)) {
+			pr_err("kern_mount?");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure we don't create mount namespace loops. */
-		if (!check_for_nsfs_mounts(old_mnt))
+		if (!check_for_nsfs_mounts(old_mnt)) {
+			pr_err("shite with nsfs");
 			return ERR_PTR(-EINVAL);
+		}
 	}
 
-	if (has_locked_children(old_mnt, path->dentry))
+	if (has_locked_children(old_mnt, path->dentry)) {
+		pr_err("has locked children");
 		return ERR_PTR(-EINVAL);
+	}
 
 	new_mnt = clone_mnt(old_mnt, path->dentry, CL_PRIVATE);
-	if (IS_ERR(new_mnt))
+	if (IS_ERR(new_mnt)) {
+		pr_err("clone_mnt failed (%ld)", PTR_ERR(new_mnt));
 		return ERR_PTR(-EINVAL);
+	}
 
 	/* Longterm mount to be removed by kern_unmount*() */
 	new_mnt->mnt_ns = MNT_NS_INTERNAL;

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 19:16:04 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 07:54:32PM +0200, Klara Modin wrote:
> 
> > > > Though now I hit another issue which I don't know if it's related or
> > > > not. I'm using an overlay mount with squashfs as lower and btrfs as
> > > > upper. The mount fails with invalid argument and I see this in the log:
> > > > 
> > > > overlayfs: failed to clone upperpath
> > > 
> > > Seeing that you already have a kernel with that thing reverted, could
> > > you check if the problem exists there?
> > 
> > Yeah, it works fine with the revert instead.
> 
> Interesting...  That message means that you've got clone_private_mount()
> returning an error; the thing is, mount passed to it has come from
> pathname lookup - it is *not* the mount created by that fc_mount() of
> vfs_create_mount() in the modified code.  That one gets passed to
> mount_subvol() and consumed there (by mount_subtree()).  All that is returned
> is root dentry; the mount passed to clone_private_mount() is created
> from scratch using dentry left by btrfs_get_tree_subvol() in its fc->root -
> see
>         dentry = mount_subvol(ctx->subvol_name, ctx->subvol_objectid, mnt);
>         ctx->subvol_name = NULL;
>         if (IS_ERR(dentry))
>                 return PTR_ERR(dentry);
> 
>         fc->root = dentry;
>         return 0;
> in the end of btrfs_get_tree_subvol().
> 
> What's more, on the overlayfs side we managed to get to
>         upper_mnt = clone_private_mount(upperpath);
>         err = PTR_ERR(upper_mnt);
>         if (IS_ERR(upper_mnt)) {
>                 pr_err("failed to clone upperpath\n");
>                 goto out;
> so the upper path had been resolved...
> 
> OK, let's try to see what clone_private_mount() is unhappy about...
> Could you try the following on top of -next + braino fix and see
> what shows up?  Another interesting thing, assuming you can get
> to shell after overlayfs mount failure, would be /proc/self/mountinfo
> contents and stat(1) output for upper path of your overlayfs mount...

It looks like the mount never succeded in the first place? It doesn't
appear in /proc/self/mountinfo at all:

2 2 0:2 / / rw - rootfs rootfs rw
24 2 0:22 / /proc rw,relatime - proc proc rw
25 2 0:23 / /sys rw,relatime - sysfs sys rw
26 2 0:6 / /dev rw,relatime - devtmpfs dev rw,size=481992k,nr_inodes=120498,mode=755
27 2 259:1 / /mnt/root-ro ro,relatime - squashfs /dev/nvme0n1 ro,errors=continue

I get the "kern_mount?" message.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 08:34:27PM +0200, Klara Modin wrote:

> > What's more, on the overlayfs side we managed to get to
> >         upper_mnt = clone_private_mount(upperpath);
> >         err = PTR_ERR(upper_mnt);
> >         if (IS_ERR(upper_mnt)) {
> >                 pr_err("failed to clone upperpath\n");
> >                 goto out;
> > so the upper path had been resolved...
> > 
> > OK, let's try to see what clone_private_mount() is unhappy about...
> > Could you try the following on top of -next + braino fix and see
> > what shows up?  Another interesting thing, assuming you can get
> > to shell after overlayfs mount failure, would be /proc/self/mountinfo
> > contents and stat(1) output for upper path of your overlayfs mount...
> 
> It looks like the mount never succeded in the first place? It doesn't
> appear in /proc/self/mountinfo at all:
> 
> 2 2 0:2 / / rw - rootfs rootfs rw
> 24 2 0:22 / /proc rw,relatime - proc proc rw
> 25 2 0:23 / /sys rw,relatime - sysfs sys rw
> 26 2 0:6 / /dev rw,relatime - devtmpfs dev rw,size=481992k,nr_inodes=120498,mode=755
> 27 2 259:1 / /mnt/root-ro ro,relatime - squashfs /dev/nvme0n1 ro,errors=continue
> 
> I get the "kern_mount?" message.

What the... actually, the comment in front of that thing makes no
sense whatsoever - it's *not* something kernel-internal; we get
there for mounts that are absolute roots of some non-anonymous
namespace; kernel-internal ones fail on if (!is_mounted(...))
just above that.

OK, the comment came from db04662e2f4f "fs: allow detached mounts
in clone_private_mount()" and it does point in an interesting
direction - commit message there speaks of overlayfs and use of
descriptors to specify layers.

Not that check_for_nsfs_mounts() (from the same commit) made any sense
there - we don't *care* about anything mounted somewhere in that mount,
since whatever's mounted on top of it does not follow into the copy
(which is what has_locked_children() call is about - in effect, in copy
you see all mountpoints that had been covered in the original)...

Oh, well - so we are seeing an absolute root of some non-anonymous
namespace there.  Or a weird detached mount claimed to belong to
some namespace, anyway.

Let's see if that's the way upperpath comes to be (and get a bit more
information on that weird mount):

diff --git a/fs/namespace.c b/fs/namespace.c
index eb990e9a668a..9b4c4afa2b29 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2480,31 +2480,52 @@ struct vfsmount *clone_private_mount(const struct path *path)
 
 	guard(rwsem_read)(&namespace_sem);
 
-	if (IS_MNT_UNBINDABLE(old_mnt))
+	if (IS_MNT_UNBINDABLE(old_mnt)) {
+		pr_err("unbindable");
 		return ERR_PTR(-EINVAL);
+	}
 
 	if (mnt_has_parent(old_mnt)) {
-		if (!check_mnt(old_mnt))
+		if (!check_mnt(old_mnt)) {
+			pr_err("mounted, but not in our namespace");
 			return ERR_PTR(-EINVAL);
+		}
 	} else {
-		if (!is_mounted(&old_mnt->mnt))
+		if (!is_mounted(&old_mnt->mnt)) {
+			pr_err("not mounted");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure this isn't something purely kernel internal. */
-		if (!is_anon_ns(old_mnt->mnt_ns))
+		if (!is_anon_ns(old_mnt->mnt_ns)) {
+			if (old_mnt == old_mnt->mnt_ns->root)
+				pr_err("absolute root");
+			else
+				pr_err("detached, but claimed to be in some ns");
+			if (check_mnt(old_mnt))
+				pr_err("our namespace, at that");
+			else
+				pr_err("some other non-anon namespace");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure we don't create mount namespace loops. */
-		if (!check_for_nsfs_mounts(old_mnt))
+		if (!check_for_nsfs_mounts(old_mnt)) {
+			pr_err("shite with nsfs");
 			return ERR_PTR(-EINVAL);
+		}
 	}
 
-	if (has_locked_children(old_mnt, path->dentry))
+	if (has_locked_children(old_mnt, path->dentry)) {
+		pr_err("has locked children");
 		return ERR_PTR(-EINVAL);
+	}
 
 	new_mnt = clone_mnt(old_mnt, path->dentry, CL_PRIVATE);
-	if (IS_ERR(new_mnt))
+	if (IS_ERR(new_mnt)) {
+		pr_err("clone_mnt failed (%ld)", PTR_ERR(new_mnt));
 		return ERR_PTR(-EINVAL);
+	}
 
 	/* Longterm mount to be removed by kern_unmount*() */
 	new_mnt->mnt_ns = MNT_NS_INTERNAL;

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 20:05:13 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 08:34:27PM +0200, Klara Modin wrote:
> 
> > > What's more, on the overlayfs side we managed to get to
> > >         upper_mnt = clone_private_mount(upperpath);
> > >         err = PTR_ERR(upper_mnt);
> > >         if (IS_ERR(upper_mnt)) {
> > >                 pr_err("failed to clone upperpath\n");
> > >                 goto out;
> > > so the upper path had been resolved...
> > > 
> > > OK, let's try to see what clone_private_mount() is unhappy about...
> > > Could you try the following on top of -next + braino fix and see
> > > what shows up?  Another interesting thing, assuming you can get
> > > to shell after overlayfs mount failure, would be /proc/self/mountinfo
> > > contents and stat(1) output for upper path of your overlayfs mount...
> > 
> > It looks like the mount never succeded in the first place? It doesn't
> > appear in /proc/self/mountinfo at all:
> > 
> > 2 2 0:2 / / rw - rootfs rootfs rw
> > 24 2 0:22 / /proc rw,relatime - proc proc rw
> > 25 2 0:23 / /sys rw,relatime - sysfs sys rw
> > 26 2 0:6 / /dev rw,relatime - devtmpfs dev rw,size=481992k,nr_inodes=120498,mode=755
> > 27 2 259:1 / /mnt/root-ro ro,relatime - squashfs /dev/nvme0n1 ro,errors=continue
> > 
> > I get the "kern_mount?" message.
> 
> What the... actually, the comment in front of that thing makes no
> sense whatsoever - it's *not* something kernel-internal; we get
> there for mounts that are absolute roots of some non-anonymous
> namespace; kernel-internal ones fail on if (!is_mounted(...))
> just above that.
> 
> OK, the comment came from db04662e2f4f "fs: allow detached mounts
> in clone_private_mount()" and it does point in an interesting
> direction - commit message there speaks of overlayfs and use of
> descriptors to specify layers.
> 
> Not that check_for_nsfs_mounts() (from the same commit) made any sense
> there - we don't *care* about anything mounted somewhere in that mount,
> since whatever's mounted on top of it does not follow into the copy
> (which is what has_locked_children() call is about - in effect, in copy
> you see all mountpoints that had been covered in the original)...
> 
> Oh, well - so we are seeing an absolute root of some non-anonymous
> namespace there.  Or a weird detached mount claimed to belong to
> some namespace, anyway.
> 
> Let's see if that's the way upperpath comes to be (and get a bit more
> information on that weird mount):
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index eb990e9a668a..9b4c4afa2b29 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2480,31 +2480,52 @@ struct vfsmount *clone_private_mount(const struct path *path)
>  
>  	guard(rwsem_read)(&namespace_sem);
>  
> -	if (IS_MNT_UNBINDABLE(old_mnt))
> +	if (IS_MNT_UNBINDABLE(old_mnt)) {
> +		pr_err("unbindable");
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	if (mnt_has_parent(old_mnt)) {
> -		if (!check_mnt(old_mnt))
> +		if (!check_mnt(old_mnt)) {
> +			pr_err("mounted, but not in our namespace");
>  			return ERR_PTR(-EINVAL);
> +		}
>  	} else {
> -		if (!is_mounted(&old_mnt->mnt))
> +		if (!is_mounted(&old_mnt->mnt)) {
> +			pr_err("not mounted");
>  			return ERR_PTR(-EINVAL);
> +		}
>  
>  		/* Make sure this isn't something purely kernel internal. */
> -		if (!is_anon_ns(old_mnt->mnt_ns))
> +		if (!is_anon_ns(old_mnt->mnt_ns)) {
> +			if (old_mnt == old_mnt->mnt_ns->root)
> +				pr_err("absolute root");
> +			else
> +				pr_err("detached, but claimed to be in some ns");
> +			if (check_mnt(old_mnt))
> +				pr_err("our namespace, at that");
> +			else
> +				pr_err("some other non-anon namespace");
>  			return ERR_PTR(-EINVAL);
> +		}
>  
>  		/* Make sure we don't create mount namespace loops. */
> -		if (!check_for_nsfs_mounts(old_mnt))
> +		if (!check_for_nsfs_mounts(old_mnt)) {
> +			pr_err("shite with nsfs");
>  			return ERR_PTR(-EINVAL);
> +		}
>  	}
>  
> -	if (has_locked_children(old_mnt, path->dentry))
> +	if (has_locked_children(old_mnt, path->dentry)) {
> +		pr_err("has locked children");
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	new_mnt = clone_mnt(old_mnt, path->dentry, CL_PRIVATE);
> -	if (IS_ERR(new_mnt))
> +	if (IS_ERR(new_mnt)) {
> +		pr_err("clone_mnt failed (%ld)", PTR_ERR(new_mnt));
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	/* Longterm mount to be removed by kern_unmount*() */
>  	new_mnt->mnt_ns = MNT_NS_INTERNAL;

I then get:

[    0.881616] absolute root
[    0.881618] our namespace, at that

In btrfs_get_tree_subvol:

	ret = vfs_get_tree(dup_fc);
	if (!ret) {
		ret = btrfs_reconfigure_for_mount(dup_fc);
		up_write(&dup_fc->root->d_sb->s_umount);
	}
	if (!ret)
		mnt = vfs_create_mount(fc);
	else
		mnt = ERR_PTR(ret);
	put_fs_context(dup_fc);

Should it perhaps be:
		mnt = vfs_create_mount(dup_fc);

If I try that it works.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 09:20:47PM +0200, Klara Modin wrote:

> I then get:
> 
> [    0.881616] absolute root
> [    0.881618] our namespace, at that

OK, so that's a combination of braino (times 2) in that patch
with quiet regression in clone_private_mount() from back in
January.

Reposted with fixes folded in and yes, you are absolutely
correct about the second 'fc' instead of 'dup_fc' in there.

As for the clone_private_mount() issues...  Christian has
taught it to allow roots of anon namespaces in addition to
mounts in our namespace, but did the tests in wrong order.

It's not a rare pattern - "do something to mount in our namespace
or the root of anon one" and for things like move_mount()
we absolutely do *not* want to allow it for root of our namespace,
so there this logics is fine - first split on whether it has
a parent, then for parented ones require the namespace to be
ours and for roots - require it to be anon.

In case of clone_private_mount(), though, there's nothing wrong
with "clone me a subtree of absolute root", so it has to be
done other way round - check if it's ours first, then in "not
ours" case check that it's a root of anon namespace.

Failing btrfs mount has ended up with upper layer pathname
pointing to initramfs directory where btrfs would've been
mounted, which had walked into that corner case.  In your
case the problem has already happened by that point, but on
a setup a-la X Terminal it would cause trouble...

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 19:16:04 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 07:54:32PM +0200, Klara Modin wrote:
> 
> > > > Though now I hit another issue which I don't know if it's related or
> > > > not. I'm using an overlay mount with squashfs as lower and btrfs as
> > > > upper. The mount fails with invalid argument and I see this in the log:
> > > > 
> > > > overlayfs: failed to clone upperpath
> > > 
> > > Seeing that you already have a kernel with that thing reverted, could
> > > you check if the problem exists there?
> > 
> > Yeah, it works fine with the revert instead.
> 
> Interesting...  That message means that you've got clone_private_mount()
> returning an error; the thing is, mount passed to it has come from
> pathname lookup - it is *not* the mount created by that fc_mount() of
> vfs_create_mount() in the modified code.  That one gets passed to
> mount_subvol() and consumed there (by mount_subtree()).  All that is returned
> is root dentry; the mount passed to clone_private_mount() is created
> from scratch using dentry left by btrfs_get_tree_subvol() in its fc->root -
> see
>         dentry = mount_subvol(ctx->subvol_name, ctx->subvol_objectid, mnt);
>         ctx->subvol_name = NULL;
>         if (IS_ERR(dentry))
>                 return PTR_ERR(dentry);
> 
>         fc->root = dentry;
>         return 0;
> in the end of btrfs_get_tree_subvol().
> 
> What's more, on the overlayfs side we managed to get to
>         upper_mnt = clone_private_mount(upperpath);
>         err = PTR_ERR(upper_mnt);
>         if (IS_ERR(upper_mnt)) {
>                 pr_err("failed to clone upperpath\n");
>                 goto out;
> so the upper path had been resolved...
> 
> OK, let's try to see what clone_private_mount() is unhappy about...
> Could you try the following on top of -next + braino fix and see
> what shows up?  Another interesting thing, assuming you can get
> to shell after overlayfs mount failure, would be /proc/self/mountinfo
> contents and stat(1) output for upper path of your overlayfs mount...

	ret = vfs_get_tree(dup_fc);
	if (!ret) {
		ret = btrfs_reconfigure_for_mount(dup_fc);
		up_write(&dup_fc->root->d_sb->s_umount);
	}
	if (!ret)
		mnt = vfs_create_mount(fc);
	else
		mnt = ERR_PTR(ret);
	put_fs_context(dup_fc);


I tried replacing fc with dup_fc in vfs_create_mount and it seems to
work.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 08:58:59PM +0200, Klara Modin wrote:
> > OK, let's try to see what clone_private_mount() is unhappy about...
> > Could you try the following on top of -next + braino fix and see
> > what shows up?  Another interesting thing, assuming you can get
> > to shell after overlayfs mount failure, would be /proc/self/mountinfo
> > contents and stat(1) output for upper path of your overlayfs mount...
> 
> 	ret = vfs_get_tree(dup_fc);
> 	if (!ret) {
> 		ret = btrfs_reconfigure_for_mount(dup_fc);
> 		up_write(&dup_fc->root->d_sb->s_umount);
> 	}
> 	if (!ret)
> 		mnt = vfs_create_mount(fc);
> 	else
> 		mnt = ERR_PTR(ret);
> 	put_fs_context(dup_fc);
> 
> 
> I tried replacing fc with dup_fc in vfs_create_mount and it seems to
> work.

*blink*

OK, I'm a blind idiot - blind for not seeing the braino duplication,
idiot for not thinking to check if the same thing has happened
more than once.

Kudos for catching that.  I still wonder what the hell got passed
to overlayfs, though - vfs_create_mount() should've hit
        if (!fc->root)
		return ERR_PTR(-EINVAL);
since fc->root definitely was NULL there.  So we should've gotten
a failing btrfs mount; fine, but that does not explain the form
of breakage you are seeing on the overlayfs side...  Actually...
is that mount attempted while still on initramfs?  Because if
it is, we are running into a separate clone_private_mount()
bug.

There's nothing to prohibit an overlayfs mount with writable layer
being a subtree of initramfs; odd, but nothing inherently wrong
with that setup.  And prior to that clone_private_mount() change
it used to be fine; we would get a private mount with root
pointing to subtree of initramfs and went on to use that.

We used to require the original mount to be in our namespace;
Christian's change added "... or root of anon namespace".
The order of checks went wrong, though.  We check for "is
it an absolute root" *first* and treat that as a discriminator
between the new and old cases.  It should be the other way
round - "is it in our namespace" should take precedence.

IOW,
	if (!check_mount(...)) { // if it's not ours...
		// ... it should be root...
		if (mnt_has_parent(...))
			fail
		// ... of anon namespace...
		if (!is_mounted(...) || !is_anon_ns(...))
			fail
		// ... and create no namespace loops -
		// or no hidden references to namespaces, period
		if (mnt_ns_loop(...)) // or, perhaps, if (mnt_ns_from_dentry(...))
			fail
	}
Anyway, that's a separate issue.

Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 20:33:05 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 08:58:59PM +0200, Klara Modin wrote:
> > > OK, let's try to see what clone_private_mount() is unhappy about...
> > > Could you try the following on top of -next + braino fix and see
> > > what shows up?  Another interesting thing, assuming you can get
> > > to shell after overlayfs mount failure, would be /proc/self/mountinfo
> > > contents and stat(1) output for upper path of your overlayfs mount...
> > 
> > 	ret = vfs_get_tree(dup_fc);
> > 	if (!ret) {
> > 		ret = btrfs_reconfigure_for_mount(dup_fc);
> > 		up_write(&dup_fc->root->d_sb->s_umount);
> > 	}
> > 	if (!ret)
> > 		mnt = vfs_create_mount(fc);
> > 	else
> > 		mnt = ERR_PTR(ret);
> > 	put_fs_context(dup_fc);
> > 
> > 
> > I tried replacing fc with dup_fc in vfs_create_mount and it seems to
> > work.
> 
> *blink*
> 
> OK, I'm a blind idiot - blind for not seeing the braino duplication,
> idiot for not thinking to check if the same thing has happened
> more than once.

No worries, thanks for taking the time to debug.

> 
> Kudos for catching that.  I still wonder what the hell got passed
> to overlayfs, though - vfs_create_mount() should've hit
>         if (!fc->root)
> 		return ERR_PTR(-EINVAL);
> since fc->root definitely was NULL there.  So we should've gotten
> a failing btrfs mount; fine, but that does not explain the form
> of breakage you are seeing on the overlayfs side...  Actually...
> is that mount attempted while still on initramfs?  Because if
> it is, we are running into a separate clone_private_mount()
> bug.
> 
> There's nothing to prohibit an overlayfs mount with writable layer
> being a subtree of initramfs; odd, but nothing inherently wrong
> with that setup.  And prior to that clone_private_mount() change
> it used to be fine; we would get a private mount with root
> pointing to subtree of initramfs and went on to use that.
> 

Yep, this is still within the initramfs. I later switch_root into the
overlay.

> We used to require the original mount to be in our namespace;
> Christian's change added "... or root of anon namespace".
> The order of checks went wrong, though.  We check for "is
> it an absolute root" *first* and treat that as a discriminator
> between the new and old cases.  It should be the other way
> round - "is it in our namespace" should take precedence.
> 
> IOW,
> 	if (!check_mount(...)) { // if it's not ours...
> 		// ... it should be root...
> 		if (mnt_has_parent(...))
> 			fail
> 		// ... of anon namespace...
> 		if (!is_mounted(...) || !is_anon_ns(...))
> 			fail
> 		// ... and create no namespace loops -
> 		// or no hidden references to namespaces, period
> 		if (mnt_ns_loop(...)) // or, perhaps, if (mnt_ns_from_dentry(...))
> 			fail
> 	}
> Anyway, that's a separate issue.

[PATCH v2] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
no need to mess with ->s_umount.
    
[fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 7121d8c7a318..75934b25ff47 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
  * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
  * work with different options work we need to keep backward compatibility.
  */
-static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
+static int btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
 	int ret = 0;
 
-	if (fc->sb_flags & SB_RDONLY)
-		return ret;
-
-	down_write(&mnt->mnt_sb->s_umount);
-	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
+	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
 		ret = btrfs_reconfigure(fc);
-	up_write(&mnt->mnt_sb->s_umount);
+
 	return ret;
 }
 
@@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
 	security_free_mnt_opts(&fc->security);
 	fc->security = NULL;
 
-	mnt = fc_mount(dup_fc);
-	if (IS_ERR(mnt)) {
-		put_fs_context(dup_fc);
-		return PTR_ERR(mnt);
+	ret = vfs_get_tree(dup_fc);
+	if (!ret) {
+		ret = btrfs_reconfigure_for_mount(dup_fc);
+		up_write(&fc->root->d_sb->s_umount);
 	}
-	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
+	if (!ret)
+		mnt = vfs_create_mount(dup_fc);
+	else
+		mnt = ERR_PTR(ret);
 	put_fs_context(dup_fc);
-	if (ret) {
-		mntput(mnt);
-		return ret;
-	}
+	if (IS_ERR(mnt))
+		return PTR_ERR(mnt);
 
 	/*
 	 * This free's ->subvol_name, because if it isn't set we have to

Re: [PATCH v2] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Klara Modin]

On 2025-05-06 20:34:05 +0100, Al Viro wrote:
> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> no need to mess with ->s_umount.
>     
> [fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7121d8c7a318..75934b25ff47 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>   * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
>   * work with different options work we need to keep backward compatibility.
>   */
> -static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
> +static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>  {
>  	int ret = 0;
>  
> -	if (fc->sb_flags & SB_RDONLY)
> -		return ret;
> -
> -	down_write(&mnt->mnt_sb->s_umount);
> -	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> +	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
>  		ret = btrfs_reconfigure(fc);
> -	up_write(&mnt->mnt_sb->s_umount);
> +
>  	return ret;
>  }
>  
> @@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
>  	security_free_mnt_opts(&fc->security);
>  	fc->security = NULL;
>  
> -	mnt = fc_mount(dup_fc);
> -	if (IS_ERR(mnt)) {
> -		put_fs_context(dup_fc);
> -		return PTR_ERR(mnt);
> +	ret = vfs_get_tree(dup_fc);
> +	if (!ret) {
> +		ret = btrfs_reconfigure_for_mount(dup_fc);

> +		up_write(&fc->root->d_sb->s_umount);

Looks like this one crept back in.

>  	}
> -	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
> +	if (!ret)
> +		mnt = vfs_create_mount(dup_fc);
> +	else
> +		mnt = ERR_PTR(ret);
>  	put_fs_context(dup_fc);
> -	if (ret) {
> -		mntput(mnt);
> -		return ret;
> -	}
> +	if (IS_ERR(mnt))
> +		return PTR_ERR(mnt);
>  
>  	/*
>  	 * This free's ->subvol_name, because if it isn't set we have to

Re: [PATCH v2] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

On Tue, May 06, 2025 at 09:52:39PM +0200, Klara Modin wrote:

> > +		up_write(&fc->root->d_sb->s_umount);
> 
> Looks like this one crept back in.

Yes ;-/  I really need to get some coffee...

[PATCH v3] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro]

[Aaarghh...]
it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
no need to mess with ->s_umount.
    
[fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 7121d8c7a318..592ed044340c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
  * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
  * work with different options work we need to keep backward compatibility.
  */
-static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
+static int btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
 	int ret = 0;
 
-	if (fc->sb_flags & SB_RDONLY)
-		return ret;
-
-	down_write(&mnt->mnt_sb->s_umount);
-	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
+	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
 		ret = btrfs_reconfigure(fc);
-	up_write(&mnt->mnt_sb->s_umount);
+
 	return ret;
 }
 
@@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
 	security_free_mnt_opts(&fc->security);
 	fc->security = NULL;
 
-	mnt = fc_mount(dup_fc);
-	if (IS_ERR(mnt)) {
-		put_fs_context(dup_fc);
-		return PTR_ERR(mnt);
+	ret = vfs_get_tree(dup_fc);
+	if (!ret) {
+		ret = btrfs_reconfigure_for_mount(dup_fc);
+		up_write(&dup_fc->root->d_sb->s_umount);
 	}
-	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
+	if (!ret)
+		mnt = vfs_create_mount(dup_fc);
+	else
+		mnt = ERR_PTR(ret);
 	put_fs_context(dup_fc);
-	if (ret) {
-		mntput(mnt);
-		return ret;
-	}
+	if (IS_ERR(mnt))
+		return PTR_ERR(mnt);
 
 	/*
 	 * This free's ->subvol_name, because if it isn't set we have to

Re: [PATCH v3] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Qu Wenruo]

在 2025/5/7 05:28, Al Viro 写道:
> [Aaarghh...]
> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> no need to mess with ->s_umount.
>      
> [fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Reviewed-by: Qu Wenruo <wqu@suse.com>
Test-by: Qu Wenruo <wqu@suse.com>

Although the commit message can be enhanced a little, I can handle it at 
merge time, no need to re-send.

Thanks,
Qu

> ---
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 7121d8c7a318..592ed044340c 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1984,17 +1984,13 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>    * btrfs or not, setting the whole super block RO.  To make per-subvolume mounting
>    * work with different options work we need to keep backward compatibility.
>    */
> -static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt)
> +static int btrfs_reconfigure_for_mount(struct fs_context *fc)
>   {
>   	int ret = 0;
>   
> -	if (fc->sb_flags & SB_RDONLY)
> -		return ret;
> -
> -	down_write(&mnt->mnt_sb->s_umount);
> -	if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY))
> +	if (!(fc->sb_flags & SB_RDONLY) && (fc->root->d_sb->s_flags & SB_RDONLY))
>   		ret = btrfs_reconfigure(fc);
> -	up_write(&mnt->mnt_sb->s_umount);
> +
>   	return ret;
>   }
>   
> @@ -2047,17 +2043,18 @@ static int btrfs_get_tree_subvol(struct fs_context *fc)
>   	security_free_mnt_opts(&fc->security);
>   	fc->security = NULL;
>   
> -	mnt = fc_mount(dup_fc);
> -	if (IS_ERR(mnt)) {
> -		put_fs_context(dup_fc);
> -		return PTR_ERR(mnt);
> +	ret = vfs_get_tree(dup_fc);
> +	if (!ret) {
> +		ret = btrfs_reconfigure_for_mount(dup_fc);
> +		up_write(&dup_fc->root->d_sb->s_umount);
>   	}
> -	ret = btrfs_reconfigure_for_mount(dup_fc, mnt);
> +	if (!ret)
> +		mnt = vfs_create_mount(dup_fc);
> +	else
> +		mnt = ERR_PTR(ret);
>   	put_fs_context(dup_fc);
> -	if (ret) {
> -		mntput(mnt);
> -		return ret;
> -	}
> +	if (IS_ERR(mnt))
> +		return PTR_ERR(mnt);
>   
>   	/*
>   	 * This free's ->subvol_name, because if it isn't set we have to
> 

Re: [PATCH v3] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[David Sterba]

On Thu, May 08, 2025 at 06:59:04PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/5/7 05:28, Al Viro 写道:
> > [Aaarghh...]
> > it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> > no need to mess with ->s_umount.
> >      
> > [fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Test-by: Qu Wenruo <wqu@suse.com>
> 
> Although the commit message can be enhanced a little, I can handle it at 
> merge time, no need to re-send.

If you're going to add the patch to for-next, please fix the subject
line and update the changelog. Thanks.

Re: [PATCH v3] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Qu Wenruo]

在 2025/6/3 17:29, David Sterba 写道:
> On Thu, May 08, 2025 at 06:59:04PM +0930, Qu Wenruo wrote:
>>
>>
>> 在 2025/5/7 05:28, Al Viro 写道:
>>> [Aaarghh...]
>>> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
>>> no need to mess with ->s_umount.
>>>       
>>> [fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
>>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>> Test-by: Qu Wenruo <wqu@suse.com>
>>
>> Although the commit message can be enhanced a little, I can handle it at
>> merge time, no need to re-send.
> 
> If you're going to add the patch to for-next, please fix the subject
> line and update the changelog. Thanks.
> 

I have merged this one to for-next just minutes ago.

However the version I pushed doesn't only have its commit 
message/subject modified, but also modified its error handling, to align 
with our error-first behavior.
(Which is much easier to read compared to the one in the patch)

So I have sent the updated version to the mail list just for reference.

Thanks,
Qu

Re: [PATCH v3] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[David Sterba]

On Tue, Jun 03, 2025 at 06:53:47PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/6/3 17:29, David Sterba 写道:
> > On Thu, May 08, 2025 at 06:59:04PM +0930, Qu Wenruo wrote:
> >>
> >>
> >> 在 2025/5/7 05:28, Al Viro 写道:
> >>> [Aaarghh...]
> >>> it's simpler to do btrfs_reconfigure_for_mount() right after vfs_get_tree() -
> >>> no need to mess with ->s_umount.
> >>>       
> >>> [fix for braino(s) folded in - kudos to Klara Modin <klarasmodin@gmail.com>]
> >>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> >>
> >> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >> Test-by: Qu Wenruo <wqu@suse.com>
> >>
> >> Although the commit message can be enhanced a little, I can handle it at
> >> merge time, no need to re-send.
> > 
> > If you're going to add the patch to for-next, please fix the subject
> > line and update the changelog. Thanks.
> > 
> 
> I have merged this one to for-next just minutes ago.
> 
> However the version I pushed doesn't only have its commit 
> message/subject modified, but also modified its error handling, to align 
> with our error-first behavior.
> (Which is much easier to read compared to the one in the patch)
> 
> So I have sent the updated version to the mail list just for reference.

Perfect, thanks.