Usermode helper executes all binaries in global "init" root context. This
doesn't allow to call to call the binary from other root (for example in a
container).
Currently, containerized NFS server requires an ability to execute a binary in
a other context, than "init" root (UMH is used for client recovery tracking).
This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
passed as a part of subprocess_info data, and a call_usermodehelper_root()
helper, which accept root as a parameter. Root path reference must be hold by
the caller, since it will be on UMH thread exit.
Signed-off-by: Stanislav Kinsbursky <[email protected]>
---
include/linux/kmod.h | 9 +++++++++
kernel/kmod.c | 41 ++++++++++++++++++++++++++++++++++-------
2 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 0555cc6..0b041c7 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -64,12 +64,21 @@ struct subprocess_info {
int (*init)(struct subprocess_info *info, struct cred *new);
void (*cleanup)(struct subprocess_info *info);
void *data;
+ struct path *root;
};
extern int
+call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
+ struct path *root);
+extern int
call_usermodehelper(char *path, char **argv, char **envp, int wait);
extern struct subprocess_info *
+call_usermodehelper_setup_root(char *path, char **argv, char **envp, gfp_t gfp_mask,
+ int (*init)(struct subprocess_info *info, struct cred *new),
+ void (*cleanup)(struct subprocess_info *), void *data,
+ struct path *root);
+extern struct subprocess_info *
call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
int (*init)(struct subprocess_info *info, struct cred *new),
void (*cleanup)(struct subprocess_info *), void *data);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 1296e72..1b41f2c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -39,6 +39,7 @@
#include <linux/rwsem.h>
#include <linux/ptrace.h>
#include <linux/async.h>
+#include <linux/fs_struct.h>
#include <asm/uaccess.h>
#include <trace/events/module.h>
@@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
*/
set_user_nice(current, 0);
+ if (sub_info->root)
+ set_fs_root(current->fs, sub_info->root);
+
retval = -ENOMEM;
new = prepare_kernel_cred(current);
if (!new)
@@ -505,7 +509,7 @@ static void helper_unlock(void)
}
/**
- * call_usermodehelper_setup - prepare to call a usermode helper
+ * call_usermodehelper_setup_root - prepare to call a usermode helper
* @path: path to usermode executable
* @argv: arg vector for process
* @envp: environment for process
@@ -513,6 +517,7 @@ static void helper_unlock(void)
* @cleanup: a cleanup function
* @init: an init function
* @data: arbitrary context sensitive data
+ * @root: fs root to swap to before binary execution
*
* Returns either %NULL on allocation failure, or a subprocess_info
* structure. This should be passed to call_usermodehelper_exec to
@@ -527,11 +532,11 @@ static void helper_unlock(void)
* Function must be runnable in either a process context or the
* context in which call_usermodehelper_exec is called.
*/
-struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
+struct subprocess_info *call_usermodehelper_setup_root(char *path, char **argv,
char **envp, gfp_t gfp_mask,
int (*init)(struct subprocess_info *info, struct cred *new),
void (*cleanup)(struct subprocess_info *info),
- void *data)
+ void *data, struct path *root)
{
struct subprocess_info *sub_info;
sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
@@ -546,9 +551,22 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
sub_info->cleanup = cleanup;
sub_info->init = init;
sub_info->data = data;
+
+ sub_info->root = root;
out:
return sub_info;
}
+EXPORT_SYMBOL(call_usermodehelper_setup_root);
+
+struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
+ char **envp, gfp_t gfp_mask,
+ int (*init)(struct subprocess_info *info, struct cred *new),
+ void (*cleanup)(struct subprocess_info *info),
+ void *data)
+{
+ return call_usermodehelper_setup_root(path, argv, envp, gfp_mask, init,
+ cleanup, data, NULL);
+}
EXPORT_SYMBOL(call_usermodehelper_setup);
/**
@@ -617,7 +635,7 @@ unlock:
EXPORT_SYMBOL(call_usermodehelper_exec);
/**
- * call_usermodehelper() - prepare and start a usermode application
+ * call_usermodehelper_root() - prepare and start a usermode application
* @path: path to usermode executable
* @argv: arg vector for process
* @envp: environment for process
@@ -625,24 +643,33 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
* when UMH_NO_WAIT don't wait at all, but you get no useful error back
* when the program couldn't be exec'ed. This makes it safe to call
* from interrupt context.
+ * @root: fs root to swap to before binary execution
*
* This function is the equivalent to use call_usermodehelper_setup() and
* call_usermodehelper_exec().
*/
-int call_usermodehelper(char *path, char **argv, char **envp, int wait)
+int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
+ struct path *root)
{
struct subprocess_info *info;
gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
- info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
- NULL, NULL, NULL);
+ info = call_usermodehelper_setup_root(path, argv, envp, gfp_mask,
+ NULL, NULL, NULL, root);
if (info == NULL)
return -ENOMEM;
return call_usermodehelper_exec(info, wait);
}
+EXPORT_SYMBOL(call_usermodehelper_root);
+
+int call_usermodehelper(char *path, char **argv, char **envp, int wait)
+{
+ return call_usermodehelper_root(path, argv, envp, wait, NULL);
+}
EXPORT_SYMBOL(call_usermodehelper);
+
static int proc_cap_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
On Mon, 20 May 2013 11:00:37 +0400
Stanislav Kinsbursky <[email protected]> wrote:
> Usermode helper executes all binaries in global "init" root context. This
> doesn't allow to call to call the binary from other root (for example in a
> container).
> Currently, containerized NFS server requires an ability to execute a binary in
> a other context, than "init" root (UMH is used for client recovery tracking).
> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
> passed as a part of subprocess_info data, and a call_usermodehelper_root()
> helper, which accept root as a parameter. Root path reference must be hold by
> the caller, since it will be on UMH thread exit.
>
I assume you mean that it will be put on thread exit.
> Signed-off-by: Stanislav Kinsbursky <[email protected]>
> ---
> include/linux/kmod.h | 9 +++++++++
> kernel/kmod.c | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 0555cc6..0b041c7 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -64,12 +64,21 @@ struct subprocess_info {
> int (*init)(struct subprocess_info *info, struct cred *new);
> void (*cleanup)(struct subprocess_info *info);
> void *data;
> + struct path *root;
> };
>
> extern int
> +call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> + struct path *root);
> +extern int
> call_usermodehelper(char *path, char **argv, char **envp, int wait);
>
> extern struct subprocess_info *
> +call_usermodehelper_setup_root(char *path, char **argv, char **envp, gfp_t gfp_mask,
> + int (*init)(struct subprocess_info *info, struct cred *new),
> + void (*cleanup)(struct subprocess_info *), void *data,
> + struct path *root);
> +extern struct subprocess_info *
> call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
> int (*init)(struct subprocess_info *info, struct cred *new),
> void (*cleanup)(struct subprocess_info *), void *data);
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 1296e72..1b41f2c 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -39,6 +39,7 @@
> #include <linux/rwsem.h>
> #include <linux/ptrace.h>
> #include <linux/async.h>
> +#include <linux/fs_struct.h>
> #include <asm/uaccess.h>
>
> #include <trace/events/module.h>
> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
> */
> set_user_nice(current, 0);
>
> + if (sub_info->root)
> + set_fs_root(current->fs, sub_info->root);
> +
> retval = -ENOMEM;
> new = prepare_kernel_cred(current);
> if (!new)
> @@ -505,7 +509,7 @@ static void helper_unlock(void)
> }
>
> /**
> - * call_usermodehelper_setup - prepare to call a usermode helper
> + * call_usermodehelper_setup_root - prepare to call a usermode helper
> * @path: path to usermode executable
> * @argv: arg vector for process
> * @envp: environment for process
> @@ -513,6 +517,7 @@ static void helper_unlock(void)
> * @cleanup: a cleanup function
> * @init: an init function
> * @data: arbitrary context sensitive data
> + * @root: fs root to swap to before binary execution
> *
> * Returns either %NULL on allocation failure, or a subprocess_info
> * structure. This should be passed to call_usermodehelper_exec to
> @@ -527,11 +532,11 @@ static void helper_unlock(void)
> * Function must be runnable in either a process context or the
> * context in which call_usermodehelper_exec is called.
> */
It would be best to spell out the need for the caller to hold a
reference in the above kerneldoc comment, since that's what people will
look at when they write new code that calls this.
> -struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> +struct subprocess_info *call_usermodehelper_setup_root(char *path, char **argv,
> char **envp, gfp_t gfp_mask,
> int (*init)(struct subprocess_info *info, struct cred *new),
> void (*cleanup)(struct subprocess_info *info),
> - void *data)
> + void *data, struct path *root)
> {
> struct subprocess_info *sub_info;
> sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
> @@ -546,9 +551,22 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> sub_info->cleanup = cleanup;
> sub_info->init = init;
> sub_info->data = data;
> +
> + sub_info->root = root;
> out:
> return sub_info;
> }
> +EXPORT_SYMBOL(call_usermodehelper_setup_root);
> +
nit: do we really need a new exported helper function here? There
aren't that many callers of call_usermodehelper_setup, so you could
just add the argument and fix up the existing callers.
> +struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> + char **envp, gfp_t gfp_mask,
> + int (*init)(struct subprocess_info *info, struct cred *new),
> + void (*cleanup)(struct subprocess_info *info),
> + void *data)
> +{
> + return call_usermodehelper_setup_root(path, argv, envp, gfp_mask, init,
> + cleanup, data, NULL);
> +}
> EXPORT_SYMBOL(call_usermodehelper_setup);
>
> /**
> @@ -617,7 +635,7 @@ unlock:
> EXPORT_SYMBOL(call_usermodehelper_exec);
>
> /**
> - * call_usermodehelper() - prepare and start a usermode application
> + * call_usermodehelper_root() - prepare and start a usermode application
> * @path: path to usermode executable
> * @argv: arg vector for process
> * @envp: environment for process
> @@ -625,24 +643,33 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
> * when UMH_NO_WAIT don't wait at all, but you get no useful error back
> * when the program couldn't be exec'ed. This makes it safe to call
> * from interrupt context.
> + * @root: fs root to swap to before binary execution
> *
> * This function is the equivalent to use call_usermodehelper_setup() and
> * call_usermodehelper_exec().
> */
I'd also spell out the need for the caller to hold an extra reference
to @root here.
> -int call_usermodehelper(char *path, char **argv, char **envp, int wait)
> +int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> + struct path *root)
> {
> struct subprocess_info *info;
> gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>
> - info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> - NULL, NULL, NULL);
> + info = call_usermodehelper_setup_root(path, argv, envp, gfp_mask,
> + NULL, NULL, NULL, root);
> if (info == NULL)
> return -ENOMEM;
>
> return call_usermodehelper_exec(info, wait);
> }
> +EXPORT_SYMBOL(call_usermodehelper_root);
> +
> +int call_usermodehelper(char *path, char **argv, char **envp, int wait)
> +{
> + return call_usermodehelper_root(path, argv, envp, wait, NULL);
> +}
> EXPORT_SYMBOL(call_usermodehelper);
>
> +
> static int proc_cap_handler(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
>
Looks reasonable otherwise, you can add my Acked-by when you fix up the
comments. Nice work...
--
Jeff Layton <[email protected]>
20.05.2013 12:42, Jeff Layton пишет:
> On Mon, 20 May 2013 11:00:37 +0400
> Stanislav Kinsbursky <[email protected]> wrote:
>
>> Usermode helper executes all binaries in global "init" root context. This
>> doesn't allow to call to call the binary from other root (for example in a
>> container).
>> Currently, containerized NFS server requires an ability to execute a binary in
>> a other context, than "init" root (UMH is used for client recovery tracking).
>> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
>> passed as a part of subprocess_info data, and a call_usermodehelper_root()
>> helper, which accept root as a parameter. Root path reference must be hold by
>> the caller, since it will be on UMH thread exit.
>>
>
> I assume you mean that it will be put on thread exit.
>
Yes, sure. Sorry.
>> Signed-off-by: Stanislav Kinsbursky <[email protected]>
>> ---
>> include/linux/kmod.h | 9 +++++++++
>> kernel/kmod.c | 41 ++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
>> index 0555cc6..0b041c7 100644
>> --- a/include/linux/kmod.h
>> +++ b/include/linux/kmod.h
>> @@ -64,12 +64,21 @@ struct subprocess_info {
>> int (*init)(struct subprocess_info *info, struct cred *new);
>> void (*cleanup)(struct subprocess_info *info);
>> void *data;
>> + struct path *root;
>> };
>>
>> extern int
>> +call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
>> + struct path *root);
>> +extern int
>> call_usermodehelper(char *path, char **argv, char **envp, int wait);
>>
>> extern struct subprocess_info *
>> +call_usermodehelper_setup_root(char *path, char **argv, char **envp, gfp_t gfp_mask,
>> + int (*init)(struct subprocess_info *info, struct cred *new),
>> + void (*cleanup)(struct subprocess_info *), void *data,
>> + struct path *root);
>> +extern struct subprocess_info *
>> call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
>> int (*init)(struct subprocess_info *info, struct cred *new),
>> void (*cleanup)(struct subprocess_info *), void *data);
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 1296e72..1b41f2c 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -39,6 +39,7 @@
>> #include <linux/rwsem.h>
>> #include <linux/ptrace.h>
>> #include <linux/async.h>
>> +#include <linux/fs_struct.h>
>> #include <asm/uaccess.h>
>>
>> #include <trace/events/module.h>
>> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
>> */
>> set_user_nice(current, 0);
>>
>> + if (sub_info->root)
>> + set_fs_root(current->fs, sub_info->root);
>> +
>> retval = -ENOMEM;
>> new = prepare_kernel_cred(current);
>> if (!new)
>> @@ -505,7 +509,7 @@ static void helper_unlock(void)
>> }
>>
>> /**
>> - * call_usermodehelper_setup - prepare to call a usermode helper
>> + * call_usermodehelper_setup_root - prepare to call a usermode helper
>> * @path: path to usermode executable
>> * @argv: arg vector for process
>> * @envp: environment for process
>> @@ -513,6 +517,7 @@ static void helper_unlock(void)
>> * @cleanup: a cleanup function
>> * @init: an init function
>> * @data: arbitrary context sensitive data
>> + * @root: fs root to swap to before binary execution
>> *
>> * Returns either %NULL on allocation failure, or a subprocess_info
>> * structure. This should be passed to call_usermodehelper_exec to
>> @@ -527,11 +532,11 @@ static void helper_unlock(void)
>> * Function must be runnable in either a process context or the
>> * context in which call_usermodehelper_exec is called.
>> */
>
> It would be best to spell out the need for the caller to hold a
> reference in the above kerneldoc comment, since that's what people will
> look at when they write new code that calls this.
>
Sound reasonable, thanks.
>> -struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> +struct subprocess_info *call_usermodehelper_setup_root(char *path, char **argv,
>> char **envp, gfp_t gfp_mask,
>> int (*init)(struct subprocess_info *info, struct cred *new),
>> void (*cleanup)(struct subprocess_info *info),
>> - void *data)
>> + void *data, struct path *root)
>> {
>> struct subprocess_info *sub_info;
>> sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
>> @@ -546,9 +551,22 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> sub_info->cleanup = cleanup;
>> sub_info->init = init;
>> sub_info->data = data;
>> +
>> + sub_info->root = root;
>> out:
>> return sub_info;
>> }
>> +EXPORT_SYMBOL(call_usermodehelper_setup_root);
>> +
>
> nit: do we really need a new exported helper function here? There
> aren't that many callers of call_usermodehelper_setup, so you could
> just add the argument and fix up the existing callers.
>
Or maybe even define call_usermodehelper_setup as a macro?
>> +struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> + char **envp, gfp_t gfp_mask,
>> + int (*init)(struct subprocess_info *info, struct cred *new),
>> + void (*cleanup)(struct subprocess_info *info),
>> + void *data)
>> +{
>> + return call_usermodehelper_setup_root(path, argv, envp, gfp_mask, init,
>> + cleanup, data, NULL);
>> +}
>> EXPORT_SYMBOL(call_usermodehelper_setup);
>>
>> /**
>> @@ -617,7 +635,7 @@ unlock:
>> EXPORT_SYMBOL(call_usermodehelper_exec);
>>
>> /**
>> - * call_usermodehelper() - prepare and start a usermode application
>> + * call_usermodehelper_root() - prepare and start a usermode application
>> * @path: path to usermode executable
>> * @argv: arg vector for process
>> * @envp: environment for process
>> @@ -625,24 +643,33 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
>> * when UMH_NO_WAIT don't wait at all, but you get no useful error back
>> * when the program couldn't be exec'ed. This makes it safe to call
>> * from interrupt context.
>> + * @root: fs root to swap to before binary execution
>> *
>> * This function is the equivalent to use call_usermodehelper_setup() and
>> * call_usermodehelper_exec().
>> */
>
> I'd also spell out the need for the caller to hold an extra reference
> to @root here.
>
Right, thanks.
>> -int call_usermodehelper(char *path, char **argv, char **envp, int wait)
>> +int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
>> + struct path *root)
>> {
>> struct subprocess_info *info;
>> gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>>
>> - info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
>> - NULL, NULL, NULL);
>> + info = call_usermodehelper_setup_root(path, argv, envp, gfp_mask,
>> + NULL, NULL, NULL, root);
>> if (info == NULL)
>> return -ENOMEM;
>>
>> return call_usermodehelper_exec(info, wait);
>> }
>> +EXPORT_SYMBOL(call_usermodehelper_root);
>> +
>> +int call_usermodehelper(char *path, char **argv, char **envp, int wait)
>> +{
>> + return call_usermodehelper_root(path, argv, envp, wait, NULL);
>> +}
>> EXPORT_SYMBOL(call_usermodehelper);
>>
>> +
>> static int proc_cap_handler(struct ctl_table *table, int write,
>> void __user *buffer, size_t *lenp, loff_t *ppos)
>> {
>>
>
> Looks reasonable otherwise, you can add my Acked-by when you fix up the
> comments. Nice work...
>
--
Best regards,
Stanislav Kinsbursky
On 05/20, Stanislav Kinsbursky wrote:
>
> Usermode helper executes all binaries in global "init" root context. This
> doesn't allow to call to call the binary from other root (for example in a
> container).
> Currently, containerized NFS server requires an ability to execute a binary in
> a other context, than "init" root (UMH is used for client recovery tracking).
> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
> passed as a part of subprocess_info data,
Why do we need the new member/arguments?
> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
> */
> set_user_nice(current, 0);
>
> + if (sub_info->root)
> + set_fs_root(current->fs, sub_info->root);
Can't subprocess_info->init() do this? You can pass root as ->data.
IOW, unless I missed something, nfs can do this without any changes
in kmod.c.
Oleg.
20.05.2013 17:57, Oleg Nesterov пишет:
> On 05/20, Stanislav Kinsbursky wrote:
>>
>> Usermode helper executes all binaries in global "init" root context. This
>> doesn't allow to call to call the binary from other root (for example in a
>> container).
>> Currently, containerized NFS server requires an ability to execute a binary in
>> a other context, than "init" root (UMH is used for client recovery tracking).
>> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
>> passed as a part of subprocess_info data,
>
> Why do we need the new member/arguments?
>
>> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
>> */
>> set_user_nice(current, 0);
>>
>> + if (sub_info->root)
>> + set_fs_root(current->fs, sub_info->root);
>
> Can't subprocess_info->init() do this? You can pass root as ->data.
>
> IOW, unless I missed something, nfs can do this without any changes
> in kmod.c.
>
> Oleg.
>
Thanks for the comment.
Yes, it definitely can. But, NFS server in the the only place. Usermode helper in
called from NFS client code and thus the same functionality is required there as well.
Moreover, set_fs_root() is not exported.
And adding an ability of a root swap to usermode helper looks quite logical. At least from the
"containers" point of view, which usually have it's own root.
--
Best regards,
Stanislav Kinsbursky
On 05/20, Stanislav Kinsbursky wrote:
>
> 20.05.2013 17:57, Oleg Nesterov пишет:
>>
>> Why do we need the new member/arguments?
>>
>>> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
>>> */
>>> set_user_nice(current, 0);
>>>
>>> + if (sub_info->root)
>>> + set_fs_root(current->fs, sub_info->root);
>>
>> Can't subprocess_info->init() do this? You can pass root as ->data.
>>
>> IOW, unless I missed something, nfs can do this without any changes
>> in kmod.c.
>>
>> Oleg.
>>
>
> Thanks for the comment.
> Yes, it definitely can. But, NFS server in the the only place. Usermode helper in
> called from NFS client code and thus the same functionality is required there as well.
Not sure I understand... OK, and why NFS client can't use the same
functionality?
> Moreover, set_fs_root() is not exported.
Then it should be exported, I think ;)
Or you can export the new helper.
> And adding an ability of a root swap to usermode helper looks quite logical. At least from the
> "containers" point of view, which usually have it's own root.
But it is not logical to uglify the code, imho.
OK, why nfs can't simply use this code
static int umh_set_fs_root(struct subprocess_info *info, struct cred *new)
{
set_fs_root(current->fs, sub_info->data);
return 0;
}
int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
struct path *root)
{
struct subprocess_info *info;
info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
umh_set_fs_root, NULL, root);
if (info == NULL)
return -ENOMEM;
return call_usermodehelper_exec(info, wait);
}
? Why do you want to add the new member, the new arguments, the new helpers?
Oleg.
On Mon, May 20, 2013 at 05:10:01PM +0200, Oleg Nesterov wrote:
> On 05/20, Stanislav Kinsbursky wrote:
> >
> > Moreover, set_fs_root() is not exported.
>
> Then it should be exported, I think ;)
Maybe--there are objections, see below.
> Or you can export the new helper.
>
> > And adding an ability of a root swap to usermode helper looks quite logical. At least from the
> > "containers" point of view, which usually have it's own root.
>
> But it is not logical to uglify the code, imho.
>
> OK, why nfs can't simply use this code
>
> static int umh_set_fs_root(struct subprocess_info *info, struct cred *new)
> {
> set_fs_root(current->fs, sub_info->data);
> return 0;
> }
>
> int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> struct path *root)
> {
>
> struct subprocess_info *info;
>
> info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> umh_set_fs_root, NULL, root);
> if (info == NULL)
> return -ENOMEM;
> return call_usermodehelper_exec(info, wait);
> }
Right, that's more or less what Stanislav proposed before:
https://patchwork.kernel.org/patch/2449081/
(though with an open-coded set_fs_root). Jeff and I asked him to try
this approach instead.
> ? Why do you want to add the new member, the new arguments, the new helpers?
- It's simpler for callers to be able to say "run this help in
that namespace" in a single line. We expect there will be
more such callers, so the mild complication of the API seems
worth it for the convenience.
- set_fs_root looks like something that shouldn't really be used
outside of a small number of well-known callers in core code.
This has come up a few times before; one I could find on a quick
search:
http://thread.gmane.org/gmane.linux.kernel/267932/focus=267998
Consensus there seems to be that users of the previously
exported set_fs_root were mostly buggy. And specifically that
adding the parameter to the usermode_helper api would be safer
than exporting set_fs_root.
--b.
You know, I am puzzled. Everything looks so clear that I can't
understand what I could miss.
On 05/20, J. Bruce Fields wrote:
>
> On Mon, May 20, 2013 at 05:10:01PM +0200, Oleg Nesterov wrote:
> > On 05/20, Stanislav Kinsbursky wrote:
> > >
> > OK, why nfs can't simply use this code
> >
> > static int umh_set_fs_root(struct subprocess_info *info, struct cred *new)
> > {
> > set_fs_root(current->fs, sub_info->data);
> > return 0;
> > }
> >
> > int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> > struct path *root)
> > {
> >
> > struct subprocess_info *info;
> >
> > info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> > umh_set_fs_root, NULL, root);
> > if (info == NULL)
> > return -ENOMEM;
> > return call_usermodehelper_exec(info, wait);
> > }
>
> Right, that's more or less what Stanislav proposed before:
>
> https://patchwork.kernel.org/patch/2449081/
>
> (though with an open-coded set_fs_root). Jeff and I asked him to try
> this approach instead.
And I still can't understand why you do not like this.
> > ? Why do you want to add the new member, the new arguments, the new helpers?
>
> - It's simpler for callers to be able to say "run this help in
> that namespace" in a single line. We expect there will be
> more such callers, so the mild complication of the API seems
> worth it for the convenience.
So call_usermodehelper_root() above doesn't look as a simple API for you?
Add it into kmod.c (or another place) and use it everywhere, why do
insist we should complicate the generic code?
What if someone wants to, say, change "nice" before running the helper?
Do you think that we need yet another change which turns
call_usermodehelper_setup_root() added by this patch into
call_usermodehelper_setup_root_nice()? And another member in sub_info?
And the "if (sub_info->nice)" check into ____call_usermodehelper() ?
> - set_fs_root looks like something that shouldn't really be used
> outside of a small number of well-known callers in core code.
OK, so do not do this. Export the new helper.
Oleg.
On Tue, May 21, 2013 at 05:28:36PM +0200, Oleg Nesterov wrote:
> You know, I am puzzled. Everything looks so clear that I can't
> understand what I could miss.
>
> On 05/20, J. Bruce Fields wrote:
> >
> > On Mon, May 20, 2013 at 05:10:01PM +0200, Oleg Nesterov wrote:
> > > On 05/20, Stanislav Kinsbursky wrote:
> > > >
> > > OK, why nfs can't simply use this code
> > >
> > > static int umh_set_fs_root(struct subprocess_info *info, struct cred *new)
> > > {
> > > set_fs_root(current->fs, sub_info->data);
> > > return 0;
> > > }
> > >
> > > int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> > > struct path *root)
> > > {
> > >
> > > struct subprocess_info *info;
> > >
> > > info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> > > umh_set_fs_root, NULL, root);
> > > if (info == NULL)
> > > return -ENOMEM;
> > > return call_usermodehelper_exec(info, wait);
> > > }
> >
> > Right, that's more or less what Stanislav proposed before:
> >
> > https://patchwork.kernel.org/patch/2449081/
> >
> > (though with an open-coded set_fs_root). Jeff and I asked him to try
> > this approach instead.
>
> And I still can't understand why you do not like this.
>
> > > ? Why do you want to add the new member, the new arguments, the new helpers?
> >
> > - It's simpler for callers to be able to say "run this help in
> > that namespace" in a single line. We expect there will be
> > more such callers, so the mild complication of the API seems
> > worth it for the convenience.
>
> So call_usermodehelper_root() above doesn't look as a simple API for you?
>
> Add it into kmod.c (or another place) and use it everywhere, why do
> insist we should complicate the generic code?
>
> What if someone wants to, say, change "nice" before running the helper?
> Do you think that we need yet another change which turns
> call_usermodehelper_setup_root() added by this patch into
> call_usermodehelper_setup_root_nice()? And another member in sub_info?
> And the "if (sub_info->nice)" check into ____call_usermodehelper() ?
>
> > - set_fs_root looks like something that shouldn't really be used
> > outside of a small number of well-known callers in core code.
>
> OK, so do not do this. Export the new helper.
You mean, export umh_set_fs_root() in the above?
That might be OK.
---b.
On 05/21, J. Bruce Fields wrote:
>
> On Tue, May 21, 2013 at 05:28:36PM +0200, Oleg Nesterov wrote:
> >
> > OK, so do not do this. Export the new helper.
>
> You mean, export umh_set_fs_root() in the above?
>
> That might be OK.
Yes, or even call_usermodehelper_root().
I didn't argue with the new helper that, yes, has the additional
argument. Although personally I think it would be better to not
add it into kmod.c (until it has more users), but I am not sure
there is a better place somewhere in nfs code. Whatever is more
convenient for you.
To me this looks like call_usermodehelper_keys().
Oleg.
Stanislav Kinsbursky <[email protected]> writes:
> Usermode helper executes all binaries in global "init" root context. This
> doesn't allow to call to call the binary from other root (for example in a
> container).
> Currently, containerized NFS server requires an ability to execute a binary in
> a other context, than "init" root (UMH is used for client recovery tracking).
> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
> passed as a part of subprocess_info data, and a call_usermodehelper_root()
> helper, which accept root as a parameter. Root path reference must be hold by
> the caller, since it will be on UMH thread exit.
This makes sense, but you should document the reference requirements in
the API. Your sentence above seems unfinished?
Thanks,
Rusty.
>
> Signed-off-by: Stanislav Kinsbursky <[email protected]>
> ---
> include/linux/kmod.h | 9 +++++++++
> kernel/kmod.c | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 0555cc6..0b041c7 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -64,12 +64,21 @@ struct subprocess_info {
> int (*init)(struct subprocess_info *info, struct cred *new);
> void (*cleanup)(struct subprocess_info *info);
> void *data;
> + struct path *root;
> };
>
> extern int
> +call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> + struct path *root);
> +extern int
> call_usermodehelper(char *path, char **argv, char **envp, int wait);
>
> extern struct subprocess_info *
> +call_usermodehelper_setup_root(char *path, char **argv, char **envp, gfp_t gfp_mask,
> + int (*init)(struct subprocess_info *info, struct cred *new),
> + void (*cleanup)(struct subprocess_info *), void *data,
> + struct path *root);
> +extern struct subprocess_info *
> call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
> int (*init)(struct subprocess_info *info, struct cred *new),
> void (*cleanup)(struct subprocess_info *), void *data);
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 1296e72..1b41f2c 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -39,6 +39,7 @@
> #include <linux/rwsem.h>
> #include <linux/ptrace.h>
> #include <linux/async.h>
> +#include <linux/fs_struct.h>
> #include <asm/uaccess.h>
>
> #include <trace/events/module.h>
> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
> */
> set_user_nice(current, 0);
>
> + if (sub_info->root)
> + set_fs_root(current->fs, sub_info->root);
> +
> retval = -ENOMEM;
> new = prepare_kernel_cred(current);
> if (!new)
> @@ -505,7 +509,7 @@ static void helper_unlock(void)
> }
>
> /**
> - * call_usermodehelper_setup - prepare to call a usermode helper
> + * call_usermodehelper_setup_root - prepare to call a usermode helper
> * @path: path to usermode executable
> * @argv: arg vector for process
> * @envp: environment for process
> @@ -513,6 +517,7 @@ static void helper_unlock(void)
> * @cleanup: a cleanup function
> * @init: an init function
> * @data: arbitrary context sensitive data
> + * @root: fs root to swap to before binary execution
> *
> * Returns either %NULL on allocation failure, or a subprocess_info
> * structure. This should be passed to call_usermodehelper_exec to
> @@ -527,11 +532,11 @@ static void helper_unlock(void)
> * Function must be runnable in either a process context or the
> * context in which call_usermodehelper_exec is called.
> */
> -struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> +struct subprocess_info *call_usermodehelper_setup_root(char *path, char **argv,
> char **envp, gfp_t gfp_mask,
> int (*init)(struct subprocess_info *info, struct cred *new),
> void (*cleanup)(struct subprocess_info *info),
> - void *data)
> + void *data, struct path *root)
> {
> struct subprocess_info *sub_info;
> sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
> @@ -546,9 +551,22 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> sub_info->cleanup = cleanup;
> sub_info->init = init;
> sub_info->data = data;
> +
> + sub_info->root = root;
> out:
> return sub_info;
> }
> +EXPORT_SYMBOL(call_usermodehelper_setup_root);
> +
> +struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
> + char **envp, gfp_t gfp_mask,
> + int (*init)(struct subprocess_info *info, struct cred *new),
> + void (*cleanup)(struct subprocess_info *info),
> + void *data)
> +{
> + return call_usermodehelper_setup_root(path, argv, envp, gfp_mask, init,
> + cleanup, data, NULL);
> +}
> EXPORT_SYMBOL(call_usermodehelper_setup);
>
> /**
> @@ -617,7 +635,7 @@ unlock:
> EXPORT_SYMBOL(call_usermodehelper_exec);
>
> /**
> - * call_usermodehelper() - prepare and start a usermode application
> + * call_usermodehelper_root() - prepare and start a usermode application
> * @path: path to usermode executable
> * @argv: arg vector for process
> * @envp: environment for process
> @@ -625,24 +643,33 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
> * when UMH_NO_WAIT don't wait at all, but you get no useful error back
> * when the program couldn't be exec'ed. This makes it safe to call
> * from interrupt context.
> + * @root: fs root to swap to before binary execution
> *
> * This function is the equivalent to use call_usermodehelper_setup() and
> * call_usermodehelper_exec().
> */
> -int call_usermodehelper(char *path, char **argv, char **envp, int wait)
> +int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
> + struct path *root)
> {
> struct subprocess_info *info;
> gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>
> - info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
> - NULL, NULL, NULL);
> + info = call_usermodehelper_setup_root(path, argv, envp, gfp_mask,
> + NULL, NULL, NULL, root);
> if (info == NULL)
> return -ENOMEM;
>
> return call_usermodehelper_exec(info, wait);
> }
> +EXPORT_SYMBOL(call_usermodehelper_root);
> +
> +int call_usermodehelper(char *path, char **argv, char **envp, int wait)
> +{
> + return call_usermodehelper_root(path, argv, envp, wait, NULL);
> +}
> EXPORT_SYMBOL(call_usermodehelper);
>
> +
> static int proc_cap_handler(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
21.05.2013 20:29, Oleg Nesterov пишет:
> On 05/21, J. Bruce Fields wrote:
>>
>> On Tue, May 21, 2013 at 05:28:36PM +0200, Oleg Nesterov wrote:
>>>
>>> OK, so do not do this. Export the new helper.
>>
>> You mean, export umh_set_fs_root() in the above?
>>
>> That might be OK.
>
> Yes, or even call_usermodehelper_root().
>
> I didn't argue with the new helper that, yes, has the additional
> argument. Although personally I think it would be better to not
> add it into kmod.c (until it has more users), but I am not sure
> there is a better place somewhere in nfs code. Whatever is more
> convenient for you.
>
> To me this looks like call_usermodehelper_keys().
>
> Oleg.
>
Thanks, Oleg. Sounds reasonable.
--
Best regards,
Stanislav Kinsbursky