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-8Vb4r2MONKCcUCVioehXQKr35eHg@xxxxxxxxxxxxxx/ >> because uid/gid parsing will fail before any assignment in most >> filesystems. >> >> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxxx> > > 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
