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

On Thu, May 3, 2018 at 12:36 AM, Alexei Starovoitov [off-list ref] 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;
+}