linux-kernel - Re: [RFC PATCH] kmod: add ability to swap root in usermode helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130521152836.GA13461@redhat.com>
Date:	Tue, 21 May 2013 17:28:36 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	"J. Bruce Fields" <bfields@...ldses.org>
Cc:	Stanislav Kinsbursky <skinsbursky@...allels.com>,
	akpm@...ux-foundation.org, jlayton@...hat.com,
	lucas.demarchi@...fusion.mobi, rusty@...tcorp.com.au,
	linux-kernel@...r.kernel.org, viro@...iv.linux.org.uk,
	bharrosh@...asas.com, devel@...nvz.org
Subject: Re: [RFC PATCH] kmod: add ability to swap root in usermode helper

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/