On Mon, Dec 8, 2014 at 2:44 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: > Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes: > >> On Mon, Dec 8, 2014 at 2:11 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: >>> >>> - Expose the knob to user space through a proc file /proc/<pid>/setgroups >>> >>> A value of 0 means the setgroups system call is disabled in the >> >> "deny" >> >>> current processes user namespace and can not be enabled in the >>> future in this user namespace. >>> >>> A value of 1 means the segtoups system call is enabled. >>> >> >> "allow" >> >>> - Descedent user namespaces inherit the value of setgroups from >> >> s/Descedent/Descendent/ > > Bah. I updated everything but the changelog comment. > >>> --- a/kernel/groups.c >>> +++ b/kernel/groups.c >>> @@ -222,6 +222,7 @@ bool may_setgroups(void) >>> * the user namespace has been established. >>> */ >>> return userns_gid_mappings_established(user_ns) && >>> + userns_setgroups_allowed(user_ns) && >>> ns_capable(user_ns, CAP_SETGID); >>> } >> >> Can you add a comment explaining the ordering? For example: > > I need to think on what I can say to make it clear. > Perhaps: /* Careful the order of these checks is important. */ > >> We need to check for a gid mapping before checking setgroups_allowed >> because an unprivileged user can create a userns with setgroups >> allowed, then disallow setgroups and add a mapping. If we check in >> the opposite order, then we have a race: we could see that setgroups >> is allowed before the user clears the bit and then see that there is a >> gid mapping after the other thread is done. > This text was actually my suggested comment text. If you put smp_rmb() in this function with a comment like that, then I think it will all make sense and be obviously correct (even with most of the other barriers removed). --Andy > Since these are independent atomic variables yes that ordering issue > seems to be the case. > > For me it was the natural ordering of the checks so I didn't even bother > to think about what happens when you reorder them. > > Eric -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html