Re: [PATCH v2 net-next 1/4] umh: introduce fork_usermode_blob() helper

[Jann Horn]

On Thu, May 3, 2018 at 12:36 AM, Alexei Starovoitov <ast@xxxxxxxxxx> wrote:
> Introduce helper:
> int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
> struct umh_info {
>        struct file *pipe_to_umh;
>        struct file *pipe_from_umh;
>        pid_t pid;
> };
>
> that GPLed kernel modules (signed or unsigned) can use it to execute part
> of its own data as swappable user mode process.
>
> The kernel will do:
> - mount "tmpfs"
> - allocate a unique file in tmpfs
> - populate that file with [data, data + len] bytes
> - user-mode-helper code will do_execve that file and, before the process
>   starts, the kernel will create two unix pipes for bidirectional
>   communication between kernel module and umh
> - close tmpfs file, effectively deleting it
> - the fork_usermode_blob will return zero on success and populate
>   'struct umh_info' with two unix pipes and the pid of the user process
>
> As the first step in the development of the bpfilter project
> the fork_usermode_blob() helper is introduced to allow user mode code
> to be invoked from a kernel module. The idea is that user mode code plus
> normal kernel module code are built as part of the kernel build
> and installed as traditional kernel module into distro specified location,
> such that from a distribution point of view, there is
> no difference between regular kernel modules and kernel modules + umh code.
> Such modules can be signed, modprobed, rmmod, etc. The use of this new helper
> by a kernel module doesn't make it any special from kernel and user space
> tooling point of view.
[...]
> +static struct vfsmount *umh_fs;
> +
> +static int init_tmpfs(void)
> +{
> +       struct file_system_type *type;
> +
> +       if (umh_fs)
> +               return 0;
> +       type = get_fs_type("tmpfs");
> +       if (!type)
> +               return -ENODEV;
> +       umh_fs = kern_mount(type);
> +       if (IS_ERR(umh_fs)) {
> +               int err = PTR_ERR(umh_fs);
> +
> +               put_filesystem(type);
> +               umh_fs = NULL;
> +               return err;
> +       }
> +       return 0;
> +}

Should init_tmpfs() be holding some sort of mutex if it's fiddling
with `umh_fs`? The current code only calls it in initcall context, but
if that ever changes and two processes try to initialize the tmpfs at
the same time, a few things could go wrong.
I guess Luis' suggestion (putting a call to init_tmpfs() in
do_basic_setup()) might be the easiest way to get rid of that problem.

> +static int alloc_tmpfs_file(size_t size, struct file **filp)
> +{
> +       struct file *file;
> +       int err;
> +
> +       err = init_tmpfs();
> +       if (err)
> +               return err;
> +       file = shmem_file_setup_with_mnt(umh_fs, "umh", size, VM_NORESERVE);
> +       if (IS_ERR(file))
> +               return PTR_ERR(file);
> +       *filp = file;
> +       return 0;
> +}