Re: [PATCH] attr: validate kuid first in chown_common

[Su Yue <glass@fydeos.io>]

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@kernel.org>
>> Cc: Seth Forshee <sforshee@digitalocean.com>
>> Fixes: b27c82e12965 ("attr: port attribute changes to new types")
>> Signed-off-by: Su Yue <glass@fydeos.io>
>> ---
> 
> 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?