Re: [PATCH v4] kernel: make groups_sort calling a responsibility group_info allocators

[Matthew Wilcox]

On Mon, Dec 11, 2017 at 11:28:06AM -0200, Thiago Rafael Becker wrote:
> +++ b/fs/nfsd/auth.c
> @@ -60,6 +60,9 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
>  				gi->gid[i] = exp->ex_anon_gid;
>  			else
>  				gi->gid[i] = rqgi->gid[i];
> +
> +			/* Should be race free as long as each thread allocates a new gi */
> +			groups_sort(gi);
>  		}

Overlong line.  Would recommend:

			/* Each thread has its own gi, so no race */

> +++ b/kernel/groups.c
> @@ -86,11 +86,13 @@ static int gid_cmp(const void *_a, const void *_b)
>  	return gid_gt(a, b) - gid_lt(a, b);
>  }
>  
> -static void groups_sort(struct group_info *group_info)
> +void groups_sort(struct group_info *group_info)
>  {
>  	sort(group_info->gid, group_info->ngroups, sizeof(*group_info->gid),
>  	     gid_cmp, NULL);
>  }
> +EXPORT_SYMBOL(groups_sort);
> +
>  

Spurious extra line

> diff --git a/net/sunrpc/svcauth_unix.c b/net/sunrpc/svcauth_unix.c
> index 740b67d..7154dab 100644
> --- a/net/sunrpc/svcauth_unix.c
> +++ b/net/sunrpc/svcauth_unix.c
> @@ -520,6 +520,12 @@ static int unix_gid_parse(struct cache_detail *cd,
>  		ug.gi->gid[i] = kgid;
>  	}
>  
> +	/* Sort the groups before inserting this entry
> +	 * into the cache to avoid future corrutpions
> +	 * by multiple simultaneous attempts to sort this
> +	 * entry.
> +	 */
> +	groups_sort(ug.gi);
>  	ugp = unix_gid_lookup(cd, uid);
>  	if (ugp) {
>  		struct cache_head *ch;

Why comment this call and not the other ones?  I appreciate this is the
call-site where you discovered the bug, but that's not going to make it
special to someone who's reading this code in ten years time.  I would
leave this comment out entirely; it's just the new way we do things.

I can't find anything else to critique; nice job.