Re: [PATCH 1/5] namespaces: move user_ns into ns_common

From: Andrew Vagin
Date: Sun Jul 24 2016 - 18:28:10 EST


On Sun, Jul 24, 2016 at 12:00:13AM -0500, Eric W. Biederman wrote:
> Andrey Vagin <avagin@xxxxxxxxxx> writes:
>
> > Every namespace has a pointer to an user namespace where is was created,
> > but they're all privately embedded in the individual namespace specific
> > structures.
> >
> > Now we are going to add an user-space interface to get an owning user
> > namespace, so it looks reasonable to move it into ns_common.
> >
> > Originally this idea was suggested by James Bottomley.
>
> I skimmed through this and I really don't like move user_ns into
> ns_common. If for no other reason that it seems to have guarantteed
> this patchset as written would not apply to my tree.

I am not insisting on this. In a second version, I will add the
get_owner operation to proc_ns_operations.

Thanks!

>
> > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> > index 8297e5b..a941b44 100644
> > --- a/include/linux/user_namespace.h
> > +++ b/include/linux/user_namespace.h
> > @@ -27,11 +27,15 @@ struct user_namespace {
> > struct uid_gid_map gid_map;
> > struct uid_gid_map projid_map;
> > atomic_t count;
> > - struct user_namespace *parent;
> > int level;
> > kuid_t owner;
> > kgid_t group;
> > - struct ns_common ns;
> > +
> > + /* ->ns.user_ns and ->parent are synonyms */
> > + union {
> > + struct user_namespace *parent;
> > + struct ns_common ns;
> > + };
> > unsigned long flags;
> >
> > /* Register of per-UID persistent keyrings for this namespace */
>
> This union is unmaintainable. It is very easy for someone to change
> ns_common and accidentially break this. The C standard does not
> allow data to be accessed as either one union member or the other.
> Which means semantically this code relies on undefined behavior, and
> the compiler can do anything in this case and gcc has sometimes been
> known to use that allowance.
>
> Eric