Move groups_sort outisde of set_groups
Subsystems (5)
v3 cover Move groups_sort outisde of set_groups
In cases where group_info is cached (e.g. sunrpc), multiplpe threads may call set_groups with a freshly created group_info cache (e.g. nfsd), and attempt to sort them simultaneously, which configures a race condition that can overwrite some groups in the cache and lead to errors. In the case of nfsd, the client was receiving EPERM if the group used to provide authorization was overwritten by this race condition.
In an email exchange with bfields, we agreed that it seems unintuitive that the groups are sorted on set_groups, and that it would be better to move the responsibility of sorting to the caller of set_groups.
These patches:
- Export groups_sort in include/linux/cred.h
- Add a call to groups_sort after the groups are inserted in group_info
- Remove the call to sort_groups from set_groups
Thiago Rafael Becker (3):
kernel: make groups_sort globally visible
kernel: Move groups_sort to the caller of set_groups.
kernel: set_groups doesn't call groups_sort anymore.
include/linux/cred.h | 1 + kernel/groups.c | 6 ++++-- kernel/uid16.c | 1 + net/sunrpc/svcauth_unix.c | 7 +++++++ 4 files changed, 13 insertions(+), 2 deletions(-)
Signature · 1 line
2.9.5
PATCH v3 1/3 kernel: make groups_sort globally visible
In preparation to move group_info sorting to the caller, make group_sort globally visible.
--- include/linux/cred.h | 1 + kernel/groups.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-)
include/linux/cred.h +1 -0
@@ -83,6 +83,7 @@ extern int set_current_groups(struct group_info *);extern void set_groups (struct cred *,struct group_info *);extern int groups_search (const struct group_info *,kgid_t );extern bool may_setgroups (void ); +extern void groups_sort (struct group_info *group_info);/* * The security context of a task
kernel/groups.c +3 -1
@@ -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); +/* a simple bsearch */ int groups_search (const struct group_info *group_info,kgid_t grp)
Signature · 1 line
2.9.5
PATCH v3 2/3 kernel: Move groups_sort to the caller of set_groups.
In reply to Thiago Rafael BeckerThe responsibility for calling groups_sort is now on the caller of set_groups.
--- kernel/groups.c | 1 + kernel/uid16.c | 1 + net/sunrpc/svcauth_unix.c | 7 +++++++ 3 files changed, 9 insertions(+)
kernel/groups.c +1 -0
@@ -208,6 +208,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)return retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);
kernel/uid16.c +1 -0
@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)return retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);
net/sunrpc/svcauth_unix.c +7 -0
@@ -20,6 +20,7 @@#include "netns.h" +void groups_sort (struct group_info *group_info);/* * AUTHUNIX and AUTHNULL credentials are both handled here.
@@ -520,6 +521,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;
Signature · 1 line
2.9.5
Re: [PATCH 2/3, v3] kernel: Move groups_sort to the caller of set_groups.
Disregard this. git reset --hard without a git stash. Sorry about it.
Quoted 63 lines · Thiago Rafael Becker
The responsibility for calling
groups_sortis now on the caller ofset_groups.Signed-off-by Thiago Rafael Becker <[email protected]>---
kernel/groups.c | 1 + kernel/uid16.c | 1 + net/sunrpc/svcauth_unix.c | 7 +++++++ 3 files changed, 9 insertions(+)kernel/groups.c +1
-0@@ -208,6 +208,7 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)return retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);kernel/uid16.c +1
-0@@ -192,6 +192,7 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist)return retval; } +groups_sort (group_info); retval =set_current_groups (group_info);put_group_info (group_info);net/sunrpc/svcauth_unix.c +7
-0@@ -20,6 +20,7 @@#include "netns.h" +void groups_sort (struct group_info *group_info);/* * AUTHUNIX and AUTHNULL credentials are both handled here. @@ -520,6 +521,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;Signature · 1 line
2.9.5
PATCH v3 3/3 kernel: set_groups doesn't call groups_sort anymore.
In reply to Thiago Rafael BeckerWhen the group_info is cached (e.g. sunrpc) there's the possibility that threads calling set_groups will attempt to do so simultaneously.
Moving the responsibility of sorting to the caller of set_groups, or in the case of nfsd, to the point where it is received from rpc.mountd avoids this issue.
Moving it to the caller has the added benifit of a slight improvement on the nfsd performance.
--- kernel/groups.c | 1 - 1 file changed, 1 deletion(-)
kernel/groups.c +0 -1
@@ -124,7 +124,6 @@ int groups_search(const struct group_info *group_info, kgid_t grp)void set_groups (struct cred *new,struct group_info *group_info) {put_group_info (new->group_info); -groups_sort (group_info);get_group_info (group_info); new->group_info = group_info; }
Signature · 1 line
2.9.5