On Tue, Aug 16, 2022 at 05:25:38PM +0800, Su Yue wrote:
> Since the commit b27c82e12965 ("attr: port attribute changes to new
> types"), chown_common stores vfs{g,u}id which converted from kuid into
> iattr::vfs{g,u}id without check of the corresponding fs mapping ids.
>
> When fchownat(2) is called with unmapped {g,u}id, now chown_common
> fails later by vfsuid_has_fsmapping in notify_change. Then it returns
> EOVERFLOW instead of EINVAL to the caller.
>
> Fix it by validating k{u,g}id whether has valid fs mapping ids in
> chown_common so it can return EINVAL early and make fchownat(2)
> behave consistently.
>
> This commit fixes fstests/generic/656.
>
> Cc: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> Cc: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>
> Fixes: b27c82e12965 ("attr: port attribute changes to new types")
> Signed-off-by: Su Yue <glass@xxxxxxxxx>
> ---
Thanks for the patch, Su!
I'm aware of this change in behavior and it is intentional. The
regression risk outside of fstests is very low. So I would prefer if we
fix the test in fstests first to check for EINVAL or EOVERFLOW.
The reason is that reporting EOVERFLOW for this case is the correct
behavior imho:
- 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.
Would you care to send something like the following:
diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
index 63297d5f..ee41110f 100644
--- a/src/vfs/idmapped-mounts.c
+++ b/src/vfs/idmapped-mounts.c
@@ -7367,7 +7367,7 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
*/
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");
/*
@@ -7457,7 +7457,7 @@ static int setattr_fix_968219708108(const struct vfstest_info *info)
*/
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");
/*
to fstests upstream?
