[PATCH] vfs/idmapped-mounts: fix unhandled EOVERFLOW in setattr_fix_968219708108

[PATCH] vfs/idmapped-mounts: fix unhandled EOVERFLOW in setattr_fix_968219708108

[Su Yue]

Since kernel commit b27c82e12965 ("attr: port attribute changes to new
types"), errno should be EOVERFLOW instead of EINVAL if fchownat(2)
called with {g,u}id whose mapping are not in fs/mount idmappings.

The errno EOVERFLOW causes failure of generic/656:
========================================================================
generic/656 3s ... [failed, exit status 1]- output mismatch (see /root/xfstests-dev/results//generic/656.out.bad)
    --- tests/generic/656.out   2022-01-03 14:18:41.049999153 +0800
    +++ /root/xfstests-dev/results//generic/656.out.bad 2022-08-16 20:09:41.679999992 +0800
    @@ -1,2 +1,4 @@
     QA output created by 656
     Silence is golden
    +idmapped-mounts.c: 7371: setattr_fix_968219708108 - Value too large for defined data type - failure: errno
    +vfstest.c: 1882: run_test - Success - failure: test that setattr works correctly
    ...
    (Run 'diff -u /root/xfstests-dev/tests/generic/656.out /root/xfstests-dev/results//generic/656.out.bad'  to see the entire diff)
Ran: generic/656
Failures: generic/656
========================================================================

As Christian Brauner explained:
"
- EINVAL should only be reported because the target {g,u}id_t has no
  mapping in the caller's idmapping, i.e. doesn't yield a valid k{g,u}id_t.
- EOVERFLOW should be reported because the target k{g,u}id_t doesn't
  have a mapping in the filesystem idmapping or mount idmapping. IOW,
  the filesystem cannot represent the intended value. The mount's
  idmapping is on a par with the filesystem idmapping and thus a failure
  to represent a vfs{g,u}id_t in the filesystem should yield EOVERFLOW.
"

EOVERFLOW is another reasonable errno so handle it after fchownat(2)
failed.

Link:https://lore.kernel.org/fstests/20220816103040.gtgg2w75tzpejas5@wittgenstein/T/#t
Signed-off-by: Su Yue <glass@fydeos.io>
---
 src/vfs/idmapped-mounts.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
index 63297d5fa43d..50550dd0ca13 100644
--- a/src/vfs/idmapped-mounts.c
+++ b/src/vfs/idmapped-mounts.c
@@ -7364,10 +7364,11 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
 		/*
 		 * The {g,u}id 0 is not mapped in this idmapped mount so this
 		 * needs to fail with EINVAL.
+		 * errno should be EOVERFLOW after kernel commit b27c82e12965.
 		 */
 		if (!fchownat(open_tree_fd, FILE1, 0, 0, AT_SYMLINK_NOFOLLOW))
 			die("failure: change ownership");
-		if (errno != EINVAL)
+		if (errno != EINVAL && errno != EOVERFLOW)
 			die("failure: errno");
 
 		/*
@@ -7454,10 +7455,11 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
 		/*
 		 * The {g,u}id 0 is not mapped in this idmapped mount so this
 		 * needs to fail with EINVAL.
+		 * errno should be EOVERFLOW after kernel commit b27c82e12965.
 		 */
 		if (!fchownat(open_tree_fd, FILE1, 0, 0, AT_SYMLINK_NOFOLLOW))
 			die("failure: change ownership");
-		if (errno != EINVAL)
+		if (errno != EINVAL && errno != EOVERFLOW)
 			die("failure: errno");
 
 		/*
-- 
2.37.1

Re: [PATCH] vfs/idmapped-mounts: fix unhandled EOVERFLOW in setattr_fix_968219708108

[Christian Brauner]

On Tue, Aug 16, 2022 at 08:15:51PM +0800, Su Yue wrote:
> Since kernel commit b27c82e12965 ("attr: port attribute changes to new
> types"), errno should be EOVERFLOW instead of EINVAL if fchownat(2)
> called with {g,u}id whose mapping are not in fs/mount idmappings.
> 
> The errno EOVERFLOW causes failure of generic/656:
> ========================================================================
> generic/656 3s ... [failed, exit status 1]- output mismatch (see /root/xfstests-dev/results//generic/656.out.bad)
>     --- tests/generic/656.out   2022-01-03 14:18:41.049999153 +0800
>     +++ /root/xfstests-dev/results//generic/656.out.bad 2022-08-16 20:09:41.679999992 +0800
>     @@ -1,2 +1,4 @@
>      QA output created by 656
>      Silence is golden
>     +idmapped-mounts.c: 7371: setattr_fix_968219708108 - Value too large for defined data type - failure: errno
>     +vfstest.c: 1882: run_test - Success - failure: test that setattr works correctly
>     ...
>     (Run 'diff -u /root/xfstests-dev/tests/generic/656.out /root/xfstests-dev/results//generic/656.out.bad'  to see the entire diff)
> Ran: generic/656
> Failures: generic/656
> ========================================================================
> 
> As Christian Brauner explained:
> "
> - EINVAL should only be reported because the target {g,u}id_t has no
>   mapping in the caller's idmapping, i.e. doesn't yield a valid k{g,u}id_t.
> - EOVERFLOW should be reported because the target k{g,u}id_t doesn't
>   have a mapping in the filesystem idmapping or mount idmapping. IOW,
>   the filesystem cannot represent the intended value. The mount's
>   idmapping is on a par with the filesystem idmapping and thus a failure
>   to represent a vfs{g,u}id_t in the filesystem should yield EOVERFLOW.
> "
> 
> EOVERFLOW is another reasonable errno so handle it after fchownat(2)
> failed.
> 
> Link:https://lore.kernel.org/fstests/20220816103040.gtgg2w75tzpejas5@wittgenstein/T/#t
> Signed-off-by: Su Yue <glass@fydeos.io>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>