[Devel] [PATCH v2 1/8] host uid and gid parameters

[Kir Kolyshkin]

On 03/22/2013 03:48 AM, Glauber Costa wrote:
> When running with an upstream Linux kernel that supports user namespaces,
> we will run the container using an unprivileged user in the system. That
> can be any user, and it serves as base to a 1:1 mapping between users in
> the container and users in the host.
>
> By default, the value 100000 will be used for both uid and gid.

So my first concern is why we have it enabled by default.
First of all, this will certainly break existing installations.

What if we have it disabled by default (unless LOCAL_UID/GID are specified),
and have vz.conf shipped with these parameters. This won't affect old 
installations
since vz.conf is marked as %config(noreplace) in vzctl.spec, while new 
installs will
use it by default.

>
> Signed-off-by: Glauber Costa <glommer at parallels.com>
> ---
>   include/res.h         |  8 ++++++++
>   include/vzctl_param.h |  3 +++
>   src/lib/config.c      | 32 ++++++++++++++++++++++++++++++++
>   3 files changed, 43 insertions(+)
>
> diff --git a/include/res.h b/include/res.h
> index e07060d..47c36da 100644
> --- a/include/res.h
> +++ b/include/res.h
> @@ -47,6 +47,12 @@ struct env_param {
>   };
>   typedef struct env_param env_param_t;
>   
> +/*
> + * When running upstream kernels, we will need host-side UID and GID. Those
> + * are configurable, but if none is specified, those are used.
> + */
> +#define VZ_DEFAULT_UID		100000
> +#define VZ_DEFAULT_GID		100000
>   typedef struct {
>   	list_head_t userpw;
>   	list_head_t nameserver;
> @@ -56,6 +62,8 @@ typedef struct {
>   	int onboot;
>   	unsigned long *bootorder;
>   	int wait;
> +	unsigned long *local_uid;
> +	unsigned long *local_gid;
>   } misc_param;
>   
>   struct mod_action;
> diff --git a/include/vzctl_param.h b/include/vzctl_param.h
> index 3034a13..e583540 100644
> --- a/include/vzctl_param.h
> +++ b/include/vzctl_param.h
> @@ -149,5 +149,8 @@
>   #define PARAM_MOUNT_OPTS	396
>   
>   
> +#define PARAM_LOCAL_UID		397
> +#define PARAM_LOCAL_GID		398
> +
>   #define PARAM_LINE		"e:p:f:t:i:l:k:a:b:n:x:h"
>   #endif
> diff --git a/src/lib/config.c b/src/lib/config.c
> index cf5f352..cc4c1cc 100644
> --- a/src/lib/config.c
> +++ b/src/lib/config.c
> @@ -128,6 +128,10 @@ static vps_config config[] = {
>   {"CONFIGFILE",	NULL, PARAM_CONFIG},
>   {"ORIGIN_SAMPLE",NULL,PARAM_CONFIG_SAMPLE},
>   {"DISABLED",	NULL, PARAM_DISABLED},
> +#ifdef HAVE_UPSTREAM
> +{"LOCAL_UID",	NULL, PARAM_LOCAL_UID},
> +{"LOCAL_GID",	NULL, PARAM_LOCAL_GID},
> +#endif
>   /* quota */
>   {"DISK_QUOTA",	NULL, PARAM_DISK_QUOTA},
>   {"DISKSPACE",	NULL, PARAM_DISKSPACE},
> @@ -1363,6 +1367,12 @@ static int store_misc(vps_param *old_p, vps_param *vps_p, vps_config *conf,
>   		ret = conf_store_strlist(conf_h, conf->name,
>   			&misc->searchdomain, 0);
>   		break;
> +	case PARAM_LOCAL_UID:
> +		ret = conf_store_ulong(conf_h, conf->name, misc->local_uid);
> +		break;
> +	case PARAM_LOCAL_GID:
> +		ret = conf_store_ulong(conf_h, conf->name, misc->local_gid);
> +		break;
>   	}
>   	return ret;
>   }
> @@ -1996,6 +2006,13 @@ static int parse(envid_t veid, vps_param *vps_p, char *val, int id)
>   	case PARAM_IPTABLES:
>   		ret = parse_iptables(&vps_p->res.env, val);
>   		break;
> +
> +	case PARAM_LOCAL_UID:
> +		conf_parse_ulong(&vps_p->res.misc.local_uid, val);
> +		break;
> +	case PARAM_LOCAL_GID:
> +		conf_parse_ulong(&vps_p->res.misc.local_gid, val);
> +		break;

While are you ignoring return value from conf_parse_ulong()?

>   	case PARAM_LOCKEDPAGES:
>   	case PARAM_PRIVVMPAGES:
>   	case PARAM_SHMPAGES:
> @@ -2697,6 +2714,8 @@ static void free_misc(misc_param *misc)
>   	FREE_P(misc->hostname)
>   	FREE_P(misc->description)
>   	FREE_P(misc->bootorder)
> +	FREE_P(misc->local_uid)
> +	FREE_P(misc->local_gid)
>   }
>   
>   static void free_net(net_param *net)
> @@ -2789,6 +2808,17 @@ void free_vps_param(vps_param *param)
>   		*dst->x = *src->x;					\
>   	}
>   
> +#define MERGE_P_DEFAULT(x, dfl)						\
> +	if ((src->x) != NULL) {						\
> +		if ((dst->x) == NULL)					\
> +			dst->x = malloc(sizeof(*(dst->x)));		\
> +		*dst->x = *src->x;					\
> +	}								\
> +	if ((dst->x) == NULL) {						\
> +		dst->x = malloc(sizeof(*(dst->x)));			\
> +		*dst->x = (unsigned long)(dfl);				\
> +	}
> +

I think you can reuse MERGE_P macro here, because your MERGE_P_DEFAULT
is logically (and textually) is exactly MERGE_P plus more.

 From the other side if we don't have built-in default we don't need 
MERGE_P_DEFAULT...

>   #define MERGE_P2(x)							\
>   	if ((src->x) != NULL) {						\
>   		if ((dst->x) == NULL)					\
> @@ -2865,6 +2895,8 @@ static void merge_misc(misc_param *dst, misc_param *src)
>   	MERGE_STR(hostname)
>   	MERGE_STR(description)
>   	MERGE_INT(onboot)
> +	MERGE_P_DEFAULT(local_uid, VZ_DEFAULT_UID)
> +	MERGE_P_DEFAULT(local_gid, VZ_DEFAULT_GID)
>   	MERGE_P(bootorder)
>   	MERGE_INT(wait)
>   }