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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
