Re: [PATCH v3] add group restriction bitmap

stsp
2024-10-01T11:44:16Z

01.10.2024 14:15, Oleg Nesterov пишет:
> I can't comment the intent, just some nits about implementation.
>
> On 09/30, Stas Sergeev wrote:
>> struct group_info {
>> refcount_t usage;
>> + unsigned int restrict_bitmap;
> Why not unsigned long?

My impl claims to support 31bit only.
Maybe use "unsigned long long" to
get like 63? Isn't "unsigned long"
arch-dependent? What would be the
benefit of an arch-dependent bitmap?

>> int groups_search(const struct group_info *group_info, kgid_t grp)
>> {
>> unsigned int left, right;
>> @@ -105,7 +108,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>> else if (gid_lt(grp, group_info->gid[mid]))
>> right = mid;
>> else
>> - return 1;
>> + return mid + 1;
> Suppose we change groups_search()
>
> --- a/kernel/groups.c
> +++ b/kernel/groups.c
> @@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> left = mid + 1;
> else if (gid_lt(grp, group_info->gid[mid]))
> right = mid;
> - else
> - return 1;
> + else {
> + bool r = mid < BITS_PER_LONG &&
> + test_bit(mid, &group_info->restrict_bitmap);
> + return r ? -1 : 1;
> + }
> }
> return 0;
> }
>
> so that it returns, say, -1 if the found grp is restricted.
>
> Then everything else can be greatly simplified, afaics...
This will mean updating all callers
of groups_search(), in_group_p(),
in_egroup_p(), vfsxx_in_group_p()
and so on. Also I am not sure if it really
helps: in_group_p() has also gid, so
if in_group_p() returns -1 for not found
and 0 for gid, then I still need to
increment the retval of groups_search(),
just as I do now.
So unless I am missing your intention,
this isn't really helping.