Re: [PATCH v3] add group restriction bitmap

[stsp]

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.