On 6/28/24 4:45 AM, Jan Kara wrote:
> On Thu 27-06-24 19:26:24, Eric Sandeen wrote:
>> Multiple filesystems take uid and gid as options, and the code to
>> create the ID from an integer and validate it is standard boilerplate
>> that can be moved into common helper functions, so do that for
>> consistency and less cut&paste.
>>
>> This also helps avoid the buggy pattern noted by Seth Jenkins at
>> https://lore.kernel.org/lkml/calxffw4bxhewxr0q5lskg-8vb4r2monkccucvioehxqkr35...@mail.gmail.com/
>> because uid/gid parsing will fail before any assignment in most
>> filesystems.
>>
>> Signed-off-by: Eric Sandeen <[email protected]>
>
> I like the idea since this seems like a nobrainer but is actually
> surprisingly subtle...
>
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index a4d6ca0b8971..24727ec34e5a 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -308,6 +308,40 @@ int fs_param_is_fd(struct p_log *log, const struct
>> fs_parameter_spec *p,
>> }
>> EXPORT_SYMBOL(fs_param_is_fd);
>>
>> +int fs_param_is_uid(struct p_log *log, const struct fs_parameter_spec *p,
>> + struct fs_parameter *param, struct fs_parse_result *result)
>> +{
>> + kuid_t uid;
>> +
>> + if (fs_param_is_u32(log, p, param, result) != 0)
>> + return fs_param_bad_value(log, param);
>> +
>> + uid = make_kuid(current_user_ns(), result->uint_32);
>
> But here is the problem: Filesystems mountable in user namespaces need to use
> fc->user_ns for resolving uids / gids (e.g. like fuse_parse_param()).
> Having helpers that work for some filesystems and are subtly broken for
> others is worse than no helpers... Or am I missing something?
Yeah, I should have pointed that out. tmpfs still does that check after the
initial trivial parsing after this change to use the basic helper:
case Opt_uid:
kuid = result.uid;
/*
* The requested uid must be representable in the
* filesystem's idmapping.
*/
if (!kuid_has_mapping(fc->user_ns, kuid))
goto bad_value;
ctx->uid = kuid;
break;
I can see your point about risks of a helper that doesn't cover all cases
though.
> And the problem with fc->user_ns is that currently __fs_parse() does not
> get fs_context as an argument... So that will need some larger work.
Yup, this was discussed a little when I sent this idea as an RFC, and the
(brief/small) consensus was that it was worth going this far for now.
Getting fc back into __fs_parse looks rather tricky and Al was not keen
on the idea, for some reason.
Thanks,
-Eric
> Honza
