Re: [PATCH 07/14] fuse: Convert to new uid/gid option parsing helpers

[Eric Sandeen <sandeen@sandeen.net>]

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@redhat.com>
>> ---
>>  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