On 2022/8/16 18:30, Christian Brauner wrote:
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!
Thanks for you quick rely.
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.
Agreed. If the errno value is intentional then a fix of fstests case is
the right.
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.
As your detailed explanation, EOVERFLOW should be aware of in real word.
Would you like to send a patch to add the above segement to man page of
fchownat(2). EOVERFLOW confused me when I first got the errno.
Would you care to send something like the following:
Just sent it.
--
Su
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?
