RE: [PATCH v3 1/3] Make call_usermodehelper_exec possible to set pid namespace

From: Zhao Lei
Date: Tue Sep 06 2016 - 05:36:01 EST

Next message: Kaur, Jasminder: "[PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage."
Previous message: Andy Yan: "Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl"
In reply to: Andrei Vagin: "Re: [PATCH v3 1/3] Make call_usermodehelper_exec possible to set pid namespace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Andrei Vagin

Thanks for review.

> -----Original Message-----
> From: Andrei Vagin [mailto:avagin@xxxxxxxxx]
> Sent: Tuesday, September 06, 2016 5:12 PM
> To: Zhao Lei <zhaolei@xxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; containers@xxxxxxxxxxxxxxxxxxxxxxxxxx; Eric W.
> Biederman <ebiederm@xxxxxxxxxxxx>; Mateusz Guzik
> <mguzik@xxxxxxxxxx>; Kamezawa Hiroyuki
> <kamezawa.hiroyu@xxxxxxxxxxxxxx>; StÃphane Graber <stgraber@xxxxxxxxxx>
> Subject: Re: [PATCH v3 1/3] Make call_usermodehelper_exec possible to set pid
> namespace
>
> On Mon, Aug 29, 2016 at 08:06:39PM +0800, Zhao Lei wrote:
> > Current call_usermodehelper_exec() can not set pid namespace for
> > the executed program, because we need addition fork to make pid
> > namespace active.
> >
> > This patch add above function for call_usermodehelper_exec().
> > When init_intermediate callback return -EAGAIN, the usermodehelper
> > will fork again to make pid namespace active, and run program
> > in the child process.
>
> I am not sure that we need to make this extra fork(). When I commented
> the previous patches, I said that we need to make fork() after
> setns(CLONE_NEWPID). But we alread have one fork(), why we can't set a
> required pidns before kernel_thread(call_usermodehelper_exec_async) and
> then switch back into the origin pidns.
>
Yes we can avoid extra fork() in above way, but I think it is better not only
setting pid ns before fork, but setting all ns before fork.

If we set pid ns before fork and set other ns after fork, the setting of ns code
will be separated into two parts, and because it is a caller's hook, it will make
the caller code complex.

If we set all ns before fork, both caller's code and the function will be simple,
the kthread will switch all ns before fork, do fork, and switch back, the child
process will inherit all ns from kthread(except for ns_for_child).

Above way seems having no problem, but I have small worry that we touched
ns of a kthread, please tell me if I losted something.

What is your opinion?

Thanks
Zhaolei

> > This function is helpful for coredump to run pipe_program in
> > specific container environment.
> >
> > Signed-off-by: Zhao Lei <zhaolei@xxxxxxxxxxxxxx>
> > ---
> > fs/coredump.c | 3 +-
> > include/linux/kmod.h | 2 +
> > init/do_mounts_initrd.c | 3 +-
> > kernel/kmod.c | 133
> ++++++++++++++++++++++++++++++++++++++------
> > lib/kobject_uevent.c | 3 +-
> > security/keys/request_key.c | 4 +-
> > 6 files changed, 127 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 281b768..ceb0ee8 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -641,7 +641,8 @@ void do_coredump(const siginfo_t *siginfo)
> > retval = -ENOMEM;
> > sub_info = call_usermodehelper_setup(helper_argv[0],
> > helper_argv, NULL, GFP_KERNEL,
> > - umh_pipe_setup, NULL, &cprm);
> > + NULL, umh_pipe_setup,
> > + NULL, &cprm);
> > if (sub_info)
> > retval = call_usermodehelper_exec(sub_info,
> > UMH_WAIT_EXEC);
> > diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> > index fcfd2bf..8fb8c0e 100644
> > --- a/include/linux/kmod.h
> > +++ b/include/linux/kmod.h
> > @@ -61,6 +61,7 @@ struct subprocess_info {
> > char **envp;
> > int wait;
> > int retval;
> > + int (*init_intermediate)(struct subprocess_info *info);
> > int (*init)(struct subprocess_info *info, struct cred *new);
> > void (*cleanup)(struct subprocess_info *info);
> > void *data;
> > @@ -71,6 +72,7 @@ call_usermodehelper(char *path, char **argv, char
> **envp, int wait);
> >
> > extern struct subprocess_info *
> > call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t
> gfp_mask,
> > + int (*init_intermediate)(struct subprocess_info *info),
> > int (*init)(struct subprocess_info *info, struct cred *new),
> > void (*cleanup)(struct subprocess_info *), void *data);
> >
> > diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
> > index a1000ca..bb5dce5 100644
> > --- a/init/do_mounts_initrd.c
> > +++ b/init/do_mounts_initrd.c
> > @@ -72,7 +72,8 @@ static void __init handle_initrd(void)
> > current->flags |= PF_FREEZER_SKIP;
> >
> > info = call_usermodehelper_setup("/linuxrc", argv, envp_init,
> > - GFP_KERNEL, init_linuxrc, NULL, NULL);
> > + GFP_KERNEL, NULL, init_linuxrc, NULL,
> > + NULL);
> > if (!info)
> > return;
> > call_usermodehelper_exec(info, UMH_WAIT_PROC);
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 0277d12..30a5802 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -91,7 +91,7 @@ static int call_modprobe(char *module_name, int wait)
> > argv[4] = NULL;
> >
> > info = call_usermodehelper_setup(modprobe_path, argv, envp,
> GFP_KERNEL,
> > - NULL, free_modprobe_argv, NULL);
> > + NULL, NULL, free_modprobe_argv, NULL);
> > if (!info)
> > goto free_module_name;
> >
> > @@ -209,14 +209,11 @@ static void umh_complete(struct subprocess_info
> *sub_info)
> > call_usermodehelper_freeinfo(sub_info);
> > }
> >
> > -/*
> > - * This is the task which runs the usermode application
> > - */
> > -static int call_usermodehelper_exec_async(void *data)
> > +static int __call_usermodehelper_exec_doexec(void *data)
> > {
> > struct subprocess_info *sub_info = data;
> > struct cred *new;
> > - int retval;
> > + int retval = 0;
> >
> > spin_lock_irq(&current->sighand->siglock);
> > flush_signal_handlers(current, 1);
> > @@ -228,10 +225,11 @@ static int call_usermodehelper_exec_async(void
> *data)
> > */
> > set_user_nice(current, 0);
> >
> > - retval = -ENOMEM;
> > new = prepare_kernel_cred(current);
> > - if (!new)
> > + if (!new) {
> > + retval = -ENOMEM;
> > goto out;
> > + }
> >
> > spin_lock(&umh_sysctl_lock);
> > new->cap_bset = cap_intersect(usermodehelper_bset, new->cap_bset);
> > @@ -248,20 +246,121 @@ static int call_usermodehelper_exec_async(void
> *data)
> > }
> >
> > commit_creds(new);
> > -
> > retval = do_execve(getname_kernel(sub_info->path),
> > - (const char __user *const __user *)sub_info->argv,
> > - (const char __user *const __user *)sub_info->envp);
> > + (const char __user *const __user *)sub_info->argv,
> > + (const char __user *const __user *)sub_info->envp);
> > +
> > out:
> > - sub_info->retval = retval;
> > + return retval;
> > +}
> > +
> > +static int call_usermodehelper_exec_doexec(void *data)
> > +{
> > + struct subprocess_info *sub_info = data;
> > + int ret = __call_usermodehelper_exec_doexec(data);
> > +
> > /*
> > - * call_usermodehelper_exec_sync() will call umh_complete
> > - * if UHM_WAIT_PROC.
> > + * If it is called in non-sync mode:
> > + * On fail:
> > + * should set sub_info->retval, call umh_complete and exit process.
> > + * On success:
> > + * should set sub_info->retval, call umh_complete and return to
> > + * user-mode program.
> > + * It it is called in sync mode:
> > + * On fail:
> > + * should set sub_info->retval and exit process, the parent process
> > + * will wait and call umh_complete()
> > + * On success:
> > + * should return to user-mode program, the parent process will wait
> > + * and get program's ret from wait(), and call umh_complete().
> > */
> > + sub_info->retval = ret;
> > +
> > if (!(sub_info->wait & UMH_WAIT_PROC))
> > umh_complete(sub_info);
> > - if (!retval)
> > +
> > + if (!ret)
> > return 0;
> > +
> > + /*
> > + * see comment in call_usermodehelper_exec_sync() before change
> > + * it to do_exit(ret).
> > + */
> > + do_exit(0);
> > +}
> > +
> > +/*
> > + * This is the task which runs the usermode application
> > + */
> > +static int call_usermodehelper_exec_async(void *data)
> > +{
> > + struct subprocess_info *sub_info = data;
> > + int ret = 0;
> > + int refork = 0;
> > +
> > + if (sub_info->init_intermediate) {
> > + ret = sub_info->init_intermediate(sub_info);
> > + switch (ret) {
> > + case 0:
> > + break;
> > + case -EAGAIN:
> > + /*
> > + * if pid namespace is changed,
> > + * we need refork to make it active.
> > + */
> > + ret = 0;
> > + refork = 1;
> > + break;
> > + default:
> > + goto out;
> > + }
> > + }
> > +
> > + if (refork) {
> > + /*
> > + * Code copyed from call_usermodehelper_exec_work() and
> > + * call_usermodehelper_exec_sync(), see comments in these
> > + * functions for detail.
> > + */
> > + pid_t pid;
> > +
> > + if (sub_info->wait & UMH_WAIT_PROC) {
> > + kernel_sigaction(SIGCHLD, SIG_DFL);
> > + pid = kernel_thread(call_usermodehelper_exec_doexec,
> > + sub_info, SIGCHLD);
> > + if (pid < 0) {
> > + ret = pid;
> > + } else {
> > + ret = -ECHILD;
> > + sys_wait4(pid, (int __user *)&ret, 0, NULL);
> > + }
> > + kernel_sigaction(SIGCHLD, SIG_IGN);
> > +
> > + sub_info->retval = ret;
> > + } else {
> > + pid = kernel_thread(call_usermodehelper_exec_doexec,
> > + sub_info, SIGCHLD);
> > + if (pid < 0) {
> > + sub_info->retval = pid;
> > + umh_complete(sub_info);
> > + }
> > + }
> > + } else {
> > + ret = __call_usermodehelper_exec_doexec(data);
> > +
> > +out:
> > + /*
> > + * see comment in call_usermodehelper_exec_doexec() for detail
> > + */
> > + sub_info->retval = ret;
> > +
> > + if (!(sub_info->wait & UMH_WAIT_PROC))
> > + umh_complete(sub_info);
> > +
> > + if (!ret)
> > + return 0;
> > + }
> > +
> > do_exit(0);
> > }
> >
> > @@ -518,6 +617,7 @@ static void helper_unlock(void)
> > */
> > struct subprocess_info *call_usermodehelper_setup(char *path, char
> **argv,
> > char **envp, gfp_t gfp_mask,
> > + int (*init_intermediate)(struct subprocess_info *info),
> > int (*init)(struct subprocess_info *info, struct cred *new),
> > void (*cleanup)(struct subprocess_info *info),
> > void *data)
> > @@ -533,6 +633,7 @@ struct subprocess_info
> *call_usermodehelper_setup(char *path, char **argv,
> > sub_info->envp = envp;
> >
> > sub_info->cleanup = cleanup;
> > + sub_info->init_intermediate = init_intermediate;
> > sub_info->init = init;
> > sub_info->data = data;
> > out:
> > @@ -619,7 +720,7 @@ int call_usermodehelper(char *path, char **argv,
> char **envp, int wait)
> > gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC :
> GFP_KERNEL;
> >
> > info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> > - NULL, NULL, NULL);
> > + NULL, NULL, NULL, NULL);
> > if (info == NULL)
> > return -ENOMEM;
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index f6c2c1e..7e571f0 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -345,7 +345,8 @@ int kobject_uevent_env(struct kobject *kobj, enum
> kobject_action action,
> > retval = -ENOMEM;
> > info = call_usermodehelper_setup(env->argv[0], env->argv,
> > env->envp, GFP_KERNEL,
> > - NULL, cleanup_uevent_env, env);
> > + NULL, NULL, cleanup_uevent_env,
> > + env);
> > if (info) {
> > retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
> > env = NULL; /* freed by cleanup_uevent_env */
> > diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> > index a29e355..087c11d 100644
> > --- a/security/keys/request_key.c
> > +++ b/security/keys/request_key.c
> > @@ -78,8 +78,8 @@ static int call_usermodehelper_keys(char *path, char
> **argv, char **envp,
> > struct subprocess_info *info;
> >
> > info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
> > - umh_keys_init, umh_keys_cleanup,
> > - session_keyring);
> > + NULL, umh_keys_init, umh_keys_cleanup,
> > + session_keyring);
> > if (!info)
> > return -ENOMEM;
> >
> > --
> > 1.8.5.1
> >
> >
> >
>

Next message: Kaur, Jasminder: "[PATCH] bonding: Prevent deletion of a bond, or the last slave from a bond, with active usage."
Previous message: Andy Yan: "Re: [PATCH 3/4] include: dt-bindings: Add more GPIO bank and index definition for rockchip pinctrl"
In reply to: Andrei Vagin: "Re: [PATCH v3 1/3] Make call_usermodehelper_exec possible to set pid namespace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]