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

[Andrew Vagin]

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