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

From: Eric W. Biederman
Date: Sun Jul 24 2016 - 01:13:29 EST


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.

> 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