Re: [linux-next:master] [fs/mnt_idmapping.c] b4291c7fd9: xfstests.generic.645.fail

Re: [linux-next:master] [fs/mnt_idmapping.c] b4291c7fd9: xfstests.generic.645.fail

[Darrick J. Wong]

On Tue, Feb 20, 2024 at 09:57:30AM +0100, Christian Brauner wrote:
> On Mon, Feb 19, 2024 at 02:55:42PM +0800, kernel test robot wrote:
> > 
> > 
> > Hello,
> > 
> > kernel test robot noticed "xfstests.generic.645.fail" on:
> > 
> > commit: b4291c7fd9e550b91b10c3d7787b9bf5be38de67 ("fs/mnt_idmapping.c: Return -EINVAL when no map is written")
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> 
> The test needs to be updated. We now explicitly fail when no map is
> written.

Has there been any progress on updating generic/645?  6.9-rc1 is out,
and Dave and I have both noticed this regressing.

--D

> > 
> > [test failed on linux-next/master d37e1e4c52bc60578969f391fb81f947c3e83118]
> > 
> > in testcase: xfstests
> > version: xfstests-x86_64-c46ca4d1-1_20240205
> > with following parameters:
> > 
> > 	disk: 4HDD
> > 	fs: f2fs
> > 	test: generic-645
> > 
> > 
> > 
> > compiler: gcc-12
> > test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory
> > 
> > (please refer to attached dmesg/kmsg for entire log/backtrace)
> > 
> > 
> > 
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <oliver.sang@intel.com>
> > | Closes: https://lore.kernel.org/oe-lkp/202402191416.17ec9160-oliver.sang@intel.com
> > 
> > 2024-02-17 15:04:19 export TEST_DIR=/fs/sda1
> > 2024-02-17 15:04:19 export TEST_DEV=/dev/sda1
> > 2024-02-17 15:04:19 export FSTYP=f2fs
> > 2024-02-17 15:04:19 export SCRATCH_MNT=/fs/scratch
> > 2024-02-17 15:04:19 mkdir /fs/scratch -p
> > 2024-02-17 15:04:19 export SCRATCH_DEV=/dev/sda4
> > 2024-02-17 15:04:19 export MKFS_OPTIONS=-f
> > 2024-02-17 15:04:19 echo generic/645
> > 2024-02-17 15:04:19 ./check generic/645
> > FSTYP         -- f2fs
> > PLATFORM      -- Linux/x86_64 lkp-skl-d03 6.8.0-rc1-00033-gb4291c7fd9e5 #1 SMP PREEMPT_DYNAMIC Sat Feb 17 22:11:35 CST 2024
> > MKFS_OPTIONS  -- -f /dev/sda4
> > MOUNT_OPTIONS -- -o acl,user_xattr /dev/sda4 /fs/scratch
> > 
> > generic/645       [failed, exit status 1]- output mismatch (see /lkp/benchmarks/xfstests/results//generic/645.out.bad)
> >     --- tests/generic/645.out	2024-02-05 17:37:40.000000000 +0000
> >     +++ /lkp/benchmarks/xfstests/results//generic/645.out.bad	2024-02-17 15:07:42.613312168 +0000
> >     @@ -1,2 +1,4 @@
> >      QA output created by 645
> >      Silence is golden
> >     +idmapped-mounts.c: 6671: nested_userns - Invalid argument - failure: sys_mount_setattr
> >     +vfstest.c: 2418: run_test - Invalid argument - failure: test that nested user namespaces behave correctly when attached to idmapped mounts
> >     ...
> >     (Run 'diff -u /lkp/benchmarks/xfstests/tests/generic/645.out /lkp/benchmarks/xfstests/results//generic/645.out.bad'  to see the entire diff)
> > Ran: generic/645
> > Failures: generic/645
> > Failed 1 of 1 tests
> > 
> > 
> > 
> > 
> > The kernel config and materials to reproduce are available at:
> > https://download.01.org/0day-ci/archive/20240219/202402191416.17ec9160-oliver.sang@intel.com
> > 
> > 
> > 
> > -- 
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> > 
> 

Re: [linux-next:master] [fs/mnt_idmapping.c] b4291c7fd9: xfstests.generic.645.fail

[Christian Brauner]

On Mon, Mar 25, 2024 at 09:58:09AM -0700, Darrick J. Wong wrote:
> On Tue, Feb 20, 2024 at 09:57:30AM +0100, Christian Brauner wrote:
> > On Mon, Feb 19, 2024 at 02:55:42PM +0800, kernel test robot wrote:
> > > 
> > > 
> > > Hello,
> > > 
> > > kernel test robot noticed "xfstests.generic.645.fail" on:
> > > 
> > > commit: b4291c7fd9e550b91b10c3d7787b9bf5be38de67 ("fs/mnt_idmapping.c: Return -EINVAL when no map is written")
> > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> > 
> > The test needs to be updated. We now explicitly fail when no map is
> > written.
> 
> Has there been any progress on updating generic/645?  6.9-rc1 is out,
> and Dave and I have both noticed this regressing.

Iirc, Taylor wanted to fix this but it seems that hasn't happened yet.
I'll ping again and if nothing's happened until tomorrow I'll send a
patch.

Re: [linux-next:master] [fs/mnt_idmapping.c] b4291c7fd9: xfstests.generic.645.fail

[Darrick J. Wong]

On Tue, Mar 26, 2024 at 12:43:27PM +0100, Christian Brauner wrote:
> On Mon, Mar 25, 2024 at 09:58:09AM -0700, Darrick J. Wong wrote:
> > On Tue, Feb 20, 2024 at 09:57:30AM +0100, Christian Brauner wrote:
> > > On Mon, Feb 19, 2024 at 02:55:42PM +0800, kernel test robot wrote:
> > > > 
> > > > 
> > > > Hello,
> > > > 
> > > > kernel test robot noticed "xfstests.generic.645.fail" on:
> > > > 
> > > > commit: b4291c7fd9e550b91b10c3d7787b9bf5be38de67 ("fs/mnt_idmapping.c: Return -EINVAL when no map is written")
> > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> > > 
> > > The test needs to be updated. We now explicitly fail when no map is
> > > written.
> > 
> > Has there been any progress on updating generic/645?  6.9-rc1 is out,
> > and Dave and I have both noticed this regressing.
> 
> Iirc, Taylor wanted to fix this but it seems that hasn't happened yet.
> I'll ping again and if nothing's happened until tomorrow I'll send a
> patch.

Ok, glad to hear that this is still on your radar.  Thank you for
following up!

--D

Re: [PATCH 2/2] vfs/idmapped_mounts.c: Change mount_setattr expected output

[Christian Brauner]

On Tue, Mar 26, 2024 at 08:33:52PM +0000, Taylor Jackson via B4 Relay wrote:
> From: Taylor Jackson <tjackson9431@gmail.com>
> 
> In kernel commit dacfd001eaf2 (“fs/mnt_idmapping.c: Return -EINVAL
> when no map is written”), the behavior of mount_setattr changed to
> return EINVAL when attempting to create an idmapped mount when using
> a user namespace with no mappings. The following commit updates the test
> to expect no mount to be created in that case. And since no mount is created,
> this commit also removes the check for overflow IDs because it does not make
> sense to check for overflow IDs for a mount that was not created.
> 
> Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
> ---

Thanks for fixing this!
Reviewed-by: Christian Brauner <brauner@kernel.org>

>  src/vfs/idmapped-mounts.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
> index 34052ca3..f4dfc3f3 100644
> --- a/src/vfs/idmapped-mounts.c
> +++ b/src/vfs/idmapped-mounts.c
> @@ -6667,7 +6667,7 @@ static int nested_userns(const struct vfstest_info *info)
>  	}
>  
>  	if (sys_mount_setattr(fd_open_tree_level4, "", AT_EMPTY_PATH,
> -			      &attr_level4, sizeof(attr_level4))) {
> +			      &attr_level4, sizeof(attr_level4)) != -1 || errno != EINVAL) {
>  		log_stderr("failure: sys_mount_setattr");
>  		goto out;
>  	}
> @@ -6706,11 +6706,6 @@ static int nested_userns(const struct vfstest_info *info)
>  			log_stderr("failure: check ownership %s", file);
>  			goto out;
>  		}
> -
> -		if (!expected_uid_gid(fd_open_tree_level4, file, 0, info->t_overflowuid, info->t_overflowgid)) {
> -			log_stderr("failure: check ownership %s", file);
> -			goto out;
> -		}
>  	}
>  
>  	/* Verify that ownership looks correct for callers in the first userns. */
> 
> -- 
> 2.34.1
> 
> 

On Tue, Mar 26, 2024 at 08:22:28AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 26, 2024 at 12:43:27PM +0100, Christian Brauner wrote:
> > On Mon, Mar 25, 2024 at 09:58:09AM -0700, Darrick J. Wong wrote:
> > > On Tue, Feb 20, 2024 at 09:57:30AM +0100, Christian Brauner wrote:
> > > > On Mon, Feb 19, 2024 at 02:55:42PM +0800, kernel test robot wrote:
> > > > > 
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > kernel test robot noticed "xfstests.generic.645.fail" on:
> > > > > 
> > > > > commit: b4291c7fd9e550b91b10c3d7787b9bf5be38de67 ("fs/mnt_idmapping.c: Return -EINVAL when no map is written")
> > > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> > > > 
> > > > The test needs to be updated. We now explicitly fail when no map is
> > > > written.
> > > 
> > > Has there been any progress on updating generic/645?  6.9-rc1 is out,
> > > and Dave and I have both noticed this regressing.
> > 
> > Iirc, Taylor wanted to fix this but it seems that hasn't happened yet.
> > I'll ping again and if nothing's happened until tomorrow I'll send a
> > patch.
> 
> Ok, glad to hear that this is still on your radar.  Thank you for
> following up!

@Darrick, Taylor sent fixes for this now (I've took the liberty to
respond to both mails combined.).

[PATCH 0/2] Patches to update xfs test "generic/645" for mount_setattr

[Taylor Jackson via B4 Relay]

The following patches fix the "generic/645" xfs test which was 
affected by the mount_setattr change in kernel commit dacfd001eaf2 
(“fs/mnt_idmapping.c: Return -EINVAL when no map is written”). 

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202402191416.17ec9160-oliver.sang@intel.com
Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
---
Taylor Jackson (2):
      vfs/idmapped_mounts.c: Incorrect array index for nested user ns
      vfs/idmapped_mounts.c: Change mount_setattr expected output

 src/vfs/idmapped-mounts.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
---
base-commit: 9b6df9a01ac8ee3f28a2a24d71e45792e21b6d48
change-id: 20240326-mount-setattr-test-90942470c820

Best regards,
-- 
Taylor Jackson <tjackson9431@gmail.com>

[PATCH 1/2] vfs/idmapped_mounts.c: Incorrect array index for nested user ns

[Taylor Jackson via B4 Relay]

From: Taylor Jackson <tjackson9431@gmail.com>

Within the vfs test for idmapped mounts, the function nested_userns()
is using an incorrect array index when attempting to set up the mapping
for the 4th nested user ns within hierarchy[4]. The correct index that
belongs to the 4th nested user ns is actually hierarchy[3].
And hierarchy[4] is reserved for the dummy entry that marks the end
of the array.

Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
---
 src/vfs/idmapped-mounts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
index 547182fe..34052ca3 100644
--- a/src/vfs/idmapped-mounts.c
+++ b/src/vfs/idmapped-mounts.c
@@ -6556,7 +6556,7 @@ static int nested_userns(const struct vfstest_info *info)
 	}
 
 	/* Don't write a mapping in the 4th userns. */
-	list_empty(&hierarchy[4].id_map);
+	list_empty(&hierarchy[3].id_map);
 
 	/* Create the actual userns hierarchy. */
 	ret = create_userns_hierarchy(hierarchy);

-- 
2.34.1

Re: [PATCH 1/2] vfs/idmapped_mounts.c: Incorrect array index for nested user ns

[Christian Brauner]

On Tue, Mar 26, 2024 at 08:33:51PM +0000, Taylor Jackson via B4 Relay wrote:
> From: Taylor Jackson <tjackson9431@gmail.com>
> 
> Within the vfs test for idmapped mounts, the function nested_userns()
> is using an incorrect array index when attempting to set up the mapping
> for the 4th nested user ns within hierarchy[4]. The correct index that
> belongs to the 4th nested user ns is actually hierarchy[3].
> And hierarchy[4] is reserved for the dummy entry that marks the end
> of the array.
> 
> Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
> ---

Ah, thanks for fixing this!
Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH 2/2] vfs/idmapped_mounts.c: Change mount_setattr expected output

[Taylor Jackson via B4 Relay]

From: Taylor Jackson <tjackson9431@gmail.com>

In kernel commit dacfd001eaf2 (“fs/mnt_idmapping.c: Return -EINVAL
when no map is written”), the behavior of mount_setattr changed to
return EINVAL when attempting to create an idmapped mount when using
a user namespace with no mappings. The following commit updates the test
to expect no mount to be created in that case. And since no mount is created,
this commit also removes the check for overflow IDs because it does not make
sense to check for overflow IDs for a mount that was not created.

Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
---
 src/vfs/idmapped-mounts.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
index 34052ca3..f4dfc3f3 100644
--- a/src/vfs/idmapped-mounts.c
+++ b/src/vfs/idmapped-mounts.c
@@ -6667,7 +6667,7 @@ static int nested_userns(const struct vfstest_info *info)
 	}
 
 	if (sys_mount_setattr(fd_open_tree_level4, "", AT_EMPTY_PATH,
-			      &attr_level4, sizeof(attr_level4))) {
+			      &attr_level4, sizeof(attr_level4)) != -1 || errno != EINVAL) {
 		log_stderr("failure: sys_mount_setattr");
 		goto out;
 	}
@@ -6706,11 +6706,6 @@ static int nested_userns(const struct vfstest_info *info)
 			log_stderr("failure: check ownership %s", file);
 			goto out;
 		}
-
-		if (!expected_uid_gid(fd_open_tree_level4, file, 0, info->t_overflowuid, info->t_overflowgid)) {
-			log_stderr("failure: check ownership %s", file);
-			goto out;
-		}
 	}
 
 	/* Verify that ownership looks correct for callers in the first userns. */

-- 
2.34.1

Re: [PATCH 2/2] vfs/idmapped_mounts.c: Change mount_setattr expected output

[Zorro Lang]

On Tue, Mar 26, 2024 at 08:33:52PM +0000, Taylor Jackson via B4 Relay wrote:
> From: Taylor Jackson <tjackson9431@gmail.com>
> 
> In kernel commit dacfd001eaf2 (“fs/mnt_idmapping.c: Return -EINVAL
> when no map is written”), the behavior of mount_setattr changed to
> return EINVAL when attempting to create an idmapped mount when using
> a user namespace with no mappings. The following commit updates the test
> to expect no mount to be created in that case. And since no mount is created,
> this commit also removes the check for overflow IDs because it does not make
> sense to check for overflow IDs for a mount that was not created.
> 
> Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
> ---

Thanks for fixing it!

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  src/vfs/idmapped-mounts.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
> index 34052ca3..f4dfc3f3 100644
> --- a/src/vfs/idmapped-mounts.c
> +++ b/src/vfs/idmapped-mounts.c
> @@ -6667,7 +6667,7 @@ static int nested_userns(const struct vfstest_info *info)
>  	}
>  
>  	if (sys_mount_setattr(fd_open_tree_level4, "", AT_EMPTY_PATH,
> -			      &attr_level4, sizeof(attr_level4))) {
> +			      &attr_level4, sizeof(attr_level4)) != -1 || errno != EINVAL) {
>  		log_stderr("failure: sys_mount_setattr");
>  		goto out;
>  	}
> @@ -6706,11 +6706,6 @@ static int nested_userns(const struct vfstest_info *info)
>  			log_stderr("failure: check ownership %s", file);
>  			goto out;
>  		}
> -
> -		if (!expected_uid_gid(fd_open_tree_level4, file, 0, info->t_overflowuid, info->t_overflowgid)) {
> -			log_stderr("failure: check ownership %s", file);
> -			goto out;
> -		}
>  	}
>  
>  	/* Verify that ownership looks correct for callers in the first userns. */
> 
> -- 
> 2.34.1
> 
> 
> 

Re: [PATCH 2/2] vfs/idmapped_mounts.c: Change mount_setattr expected output

[Zorro Lang]

On Tue, Mar 26, 2024 at 08:33:52PM +0000, Taylor Jackson via B4 Relay wrote:
> From: Taylor Jackson <tjackson9431@gmail.com>
> 
> In kernel commit dacfd001eaf2 (“fs/mnt_idmapping.c: Return -EINVAL
> when no map is written”), the behavior of mount_setattr changed to
> return EINVAL when attempting to create an idmapped mount when using
> a user namespace with no mappings. The following commit updates the test
> to expect no mount to be created in that case. And since no mount is created,
> this commit also removes the check for overflow IDs because it does not make
> sense to check for overflow IDs for a mount that was not created.
> 
> Signed-off-by: Taylor Jackson <tjackson9431@gmail.com>
> ---
>  src/vfs/idmapped-mounts.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
> index 34052ca3..f4dfc3f3 100644
> --- a/src/vfs/idmapped-mounts.c
> +++ b/src/vfs/idmapped-mounts.c
> @@ -6667,7 +6667,7 @@ static int nested_userns(const struct vfstest_info *info)
>  	}
>  
>  	if (sys_mount_setattr(fd_open_tree_level4, "", AT_EMPTY_PATH,
> -			      &attr_level4, sizeof(attr_level4))) {
> +			      &attr_level4, sizeof(attr_level4)) != -1 || errno != EINVAL) {

Hi Taylor,

This patch has been merged, but I found it causes g/645 fail on old kernel[1].
Is it expected?

If it's an expected failure on kernel without dacfd001eaf2 (“fs/mnt_idmapping.c:
Return -EINVAL when no map is written”), we'd better to add this commit to
generic/645 as a hint, to avoid others feel they hit a regression.

Thanks,
Zorro

[1]
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 sheep-3 5.14.0-xxx.el9 #1 SMP PREEMPT_DYNAMIC Fri Apr 5 12:21:04 EDT 2024
MKFS_OPTIONS  -- -f -m crc=1,finobt=1,reflink=1,rmapbt=0,bigtime=1,inobtcount=1 /dev/vda3
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vda3 /mnt/xfstests/scratch

generic/645       [failed, exit status 1]- output mismatch (see /var/lib/xfstests/results//generic/645.out.bad)
    --- tests/generic/645.out	2024-04-08 07:27:51.432955372 +0200
    +++ /var/lib/xfstests/results//generic/645.out.bad	2024-04-08 07:31:49.811581285 +0200
    @@ -1,2 +1,4 @@
     QA output created by 645
     Silence is golden
    +idmapped-mounts.c: 6671: nested_userns - Success - failure: sys_mount_setattr
    +vfstest.c: 2418: run_test - Success - failure: test that nested user namespaces behave correctly when attached to idmapped mounts
    ...
    (Run 'diff -u /var/lib/xfstests/tests/generic/645.out /var/lib/xfstests/results//generic/645.out.bad'  to see the entire diff)
Ran: generic/645
Failures: generic/645
Failed 1 of 1 tests

>  		log_stderr("failure: sys_mount_setattr");
>  		goto out;
>  	}
> @@ -6706,11 +6706,6 @@ static int nested_userns(const struct vfstest_info *info)
>  			log_stderr("failure: check ownership %s", file);
>  			goto out;
>  		}
> -
> -		if (!expected_uid_gid(fd_open_tree_level4, file, 0, info->t_overflowuid, info->t_overflowgid)) {
> -			log_stderr("failure: check ownership %s", file);
> -			goto out;
> -		}
>  	}
>  
>  	/* Verify that ownership looks correct for callers in the first userns. */
> 
> -- 
> 2.34.1
> 
> 
>