On 6/28/24 7:07 AM, Christian Brauner wrote:
> I think you accidently Cced the wrong Miklos. :)
>
> On Thu, Jun 27, 2024 at 07:33:43PM GMT, Eric Sandeen wrote:
>> Convert to new uid/gid option parsing helpers
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>> fs/fuse/inode.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 99e44ea7d875..1ac528bcdb3c 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -740,8 +740,8 @@ static const struct fs_parameter_spec fuse_fs_parameters[] = {
>> fsparam_string ("source", OPT_SOURCE),
>> fsparam_u32 ("fd", OPT_FD),
>> fsparam_u32oct ("rootmode", OPT_ROOTMODE),
>> - fsparam_u32 ("user_id", OPT_USER_ID),
>> - fsparam_u32 ("group_id", OPT_GROUP_ID),
>> + fsparam_uid ("user_id", OPT_USER_ID),
>> + fsparam_gid ("group_id", OPT_GROUP_ID),
>> fsparam_flag ("default_permissions", OPT_DEFAULT_PERMISSIONS),
>> fsparam_flag ("allow_other", OPT_ALLOW_OTHER),
>> fsparam_u32 ("max_read", OPT_MAX_READ),
>> @@ -799,16 +799,12 @@ static int fuse_parse_param(struct fs_context *fsc, struct fs_parameter *param)
>> break;
>>
>> case OPT_USER_ID:
>> - ctx->user_id = make_kuid(fsc->user_ns, result.uint_32);
>> - if (!uid_valid(ctx->user_id))
>> - return invalfc(fsc, "Invalid user_id");
>> + ctx->user_id = result.uid;
>
> So fsc->user_ns will record the namespaces at fsopen() time which can be
> different from the namespace used at fsconfig() time. This was done when
> fuse was ported to the new mount api.
>
> It has the same potential issues that Seth pointed out so I think your
> patch is correct. But I also think we might need the same verification
> that tmpfs is doing to verify that the uid/gid we're using can actually
> be represented in the fsc->user_ns.
Hm yeah, so that's a current bug in fuse, right? (it /would/ be really
nice to be able to spot FS_USERNS_MOUNT in the helper, and do the right
thing in all cases)
> So maybe there should be a separate patch that converts fuse to rely on
> make_k*id(current_user_ns()) + k*id_has_mapping() and then these patches
> on top?
Ok, I guess the idea is that that patch could land more quickly than this
treewide-ish change (and would be cherry-pickable -stable material), to fix
the bug, and then make this change later. Seems fair.
Thanks,
-Eric
