[Devel] [PATCH 1/6] host uid and gid parameters

[Kir Kolyshkin]

Looks good in general. See questions below.

On 03/11/2013 04:01 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.
>
> 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..548cde7 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..c357107 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, (unsigned long *)&misc->local_uid);
> +		break;
> +	case PARAM_LOCAL_GID:
> +		ret = conf_store_ulong(conf_h, conf->name, (unsigned long *)&misc->local_gid);
> +		break;
>   	}
>   	return ret;
>   }
> @@ -1996,6 +2006,24 @@ 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:
> +		ret = parse_ul(val, &vps_p->res.misc.local_uid);

1 Are we OK with the fact that the biggest number returned from 
parse_ul() is LONG_MAX and not ULONG_MAX?

2 We use conf_parse_ulong() and pointer to unsigned long in similar 
places in order to detect situations such as unset value or the value 
set twice in config. If you don't want that, I guess it's fine to use 
parse_ul() directly...

> +		if (ret != 0)
> +			break;
> +
> +		if (vps_p->res.misc.local_uid == 0)
> +			vps_p->res.misc.local_uid = VZ_DEFAULT_UID;

So here you deliberately disable possibility to use 0 as an offset, 
making 0 mean "use default". RIght?

> +		break;
> +
> +	case PARAM_LOCAL_GID:
> +		ret = parse_ul(val, &vps_p->res.misc.local_gid);
> +		if (ret != 0)
> +			break;
> +
> +		if (vps_p->res.misc.local_gid == 0)
> +			vps_p->res.misc.local_gid = VZ_DEFAULT_GID;
> +		break;
>   	case PARAM_LOCKEDPAGES:
>   	case PARAM_PRIVVMPAGES:
>   	case PARAM_SHMPAGES:
> @@ -2565,6 +2593,8 @@ vps_param *init_vps_param()
>   	param->res.meminfo.mode = -1;
>   	param->res.io.ioprio = -1;
>   	param->opt.mode = -1;
> +	param->res.misc.local_uid = VZ_DEFAULT_UID;
> +	param->res.misc.local_gid = VZ_DEFAULT_GID;
>   
>   	return param;
>   }
> @@ -2865,6 +2895,8 @@ static void merge_misc(misc_param *dst, misc_param *src)
>   	MERGE_STR(hostname)
>   	MERGE_STR(description)
>   	MERGE_INT(onboot)
> +	MERGE_INT(local_uid)
> +	MERGE_INT(local_gid)
>   	MERGE_P(bootorder)
>   	MERGE_INT(wait)
>   }