diff options
author | Eric W. Biederman <ebiederm@xmission.com> | 2020-07-08 07:48:48 +0300 |
---|---|---|
committer | Eric W. Biederman <ebiederm@xmission.com> | 2020-07-08 07:52:29 +0300 |
commit | f06b71fe4d4cd0a4ad7e183b777564f696f6bb36 (patch) | |
tree | ab7c8a13fa7b10a9586ca1104534a009b2e41333 /net | |
parent | b3a9e3b9622ae10064826dccb4f7a52bd88c7407 (diff) | |
parent | 33c326014fe69304244868cc793c2c77be533125 (diff) | |
download | linux-f06b71fe4d4cd0a4ad7e183b777564f696f6bb36.tar.xz |
Make the user mode driver code a better citizen
This is the third round of my changeset to split the user mode driver
code from the user mode helper code, and to make the code use common
facilities to get things done instead of recreating them just
for the user mode driver code.
I have split the changes into small enough pieces so they should be
easily readable and testable.
The changes lean into the preexisting interfaces in the kernel and
remove special cases for user mode driver code in favor of solutions
that don't need special cases. This results in smaller code with fewer
bugs.
At a practical level this removes the maintenance burden of the user
mode drivers from the user mode helper code and from exec as the special
cases are removed.
Similarly the LSM interaction bugs are fixed by not having unnecessary
special cases for user mode drivers.
I have tested thes changes by booting with the code compiled in and
by killing "bpfilter_umh" and "running iptables -vnL" to restart
the userspace driver, also by running "while true; do iptables -L;rmmod
bpfilter; done" to verify the module load and unload work properly.
I have compiled tested each change with and without CONFIG_BPFILTER
enabled.
From v2 to v3 I have made two siginficant changes.
- I factored thread_group_exit out of pidfd_poll to allow the test
to be used by the bpfilter code.
- I renamed umd.c and umd.h to usermode_driver.c and usermode_driver.h
respectively.
I made a few very small changes from v1 to v2:
- Updated the function name in a comment when the function is renamed
- Moved some more code so that the the !CONFIG_BPFILTER case continues
to compile when I moved the code into umd.c
- A fix for the module loading case to really flush the file descriptor.
- Removed split_argv entirely from fork_usermode_driver.
There was nothing to split so it was just confusing.
Please let me know if you see any bugs. Once the code review is
finished I plan to place the code in a non-rebasing branch
so I can pull it into my tree and so it can also be pulled into
the bpf-next tree.
v1: https://lkml.kernel.org/r/87pn9mgfc2.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87bll17ili.fsf_-_@x220.int.ebiederm.org
Eric W. Biederman (16):
umh: Capture the pid in umh_pipe_setup
umh: Move setting PF_UMH into umh_pipe_setup
umh: Rename the user mode driver helpers for clarity
umh: Remove call_usermodehelper_setup_file.
umh: Separate the user mode driver and the user mode helper support
umd: For clarity rename umh_info umd_info
umd: Rename umd_info.cmdline umd_info.driver_name
umd: Transform fork_usermode_blob into fork_usermode_driver
umh: Stop calling do_execve_file
exec: Remove do_execve_file
bpfilter: Move bpfilter_umh back into init data
umd: Track user space drivers with struct pid
exit: Factor thread_group_exited out of pidfd_poll
bpfilter: Take advantage of the facilities of struct pid
umd: Remove exit_umh
umd: Stop using split_argv
Link: https://lkml.kernel.org/r/87y2o1swee.fsf_-_@x220.int.ebiederm.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Diffstat (limited to 'net')
-rw-r--r-- | net/bpfilter/bpfilter_kern.c | 38 | ||||
-rw-r--r-- | net/bpfilter/bpfilter_umh_blob.S | 2 | ||||
-rw-r--r-- | net/ipv4/bpfilter/sockopt.c | 20 |
3 files changed, 31 insertions, 29 deletions
diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c index c0f0990f30b6..9616fb7defeb 100644 --- a/net/bpfilter/bpfilter_kern.c +++ b/net/bpfilter/bpfilter_kern.c @@ -15,15 +15,13 @@ extern char bpfilter_umh_end; static void shutdown_umh(void) { - struct task_struct *tsk; + struct umd_info *info = &bpfilter_ops.info; + struct pid *tgid = info->tgid; - if (bpfilter_ops.stop) - return; - - tsk = get_pid_task(find_vpid(bpfilter_ops.info.pid), PIDTYPE_PID); - if (tsk) { - send_sig(SIGKILL, tsk, 1); - put_task_struct(tsk); + if (tgid) { + kill_pid(tgid, SIGKILL, 1); + wait_event(tgid->wait_pidfd, thread_group_exited(tgid)); + bpfilter_umh_cleanup(info); } } @@ -48,7 +46,7 @@ static int __bpfilter_process_sockopt(struct sock *sk, int optname, req.cmd = optname; req.addr = (long __force __user)optval; req.len = optlen; - if (!bpfilter_ops.info.pid) + if (!bpfilter_ops.info.tgid) goto out; n = __kernel_write(bpfilter_ops.info.pipe_to_umh, &req, sizeof(req), &pos); @@ -77,13 +75,10 @@ static int start_umh(void) int err; /* fork usermode process */ - err = fork_usermode_blob(&bpfilter_umh_start, - &bpfilter_umh_end - &bpfilter_umh_start, - &bpfilter_ops.info); + err = fork_usermode_driver(&bpfilter_ops.info); if (err) return err; - bpfilter_ops.stop = false; - pr_info("Loaded bpfilter_umh pid %d\n", bpfilter_ops.info.pid); + pr_info("Loaded bpfilter_umh pid %d\n", pid_nr(bpfilter_ops.info.tgid)); /* health check that usermode process started correctly */ if (__bpfilter_process_sockopt(NULL, 0, NULL, 0, 0) != 0) { @@ -98,18 +93,21 @@ static int __init load_umh(void) { int err; + err = umd_load_blob(&bpfilter_ops.info, + &bpfilter_umh_start, + &bpfilter_umh_end - &bpfilter_umh_start); + if (err) + return err; + mutex_lock(&bpfilter_ops.lock); - if (!bpfilter_ops.stop) { - err = -EFAULT; - goto out; - } err = start_umh(); if (!err && IS_ENABLED(CONFIG_INET)) { bpfilter_ops.sockopt = &__bpfilter_process_sockopt; bpfilter_ops.start = &start_umh; } -out: mutex_unlock(&bpfilter_ops.lock); + if (err) + umd_unload_blob(&bpfilter_ops.info); return err; } @@ -122,6 +120,8 @@ static void __exit fini_umh(void) bpfilter_ops.sockopt = NULL; } mutex_unlock(&bpfilter_ops.lock); + + umd_unload_blob(&bpfilter_ops.info); } module_init(load_umh); module_exit(fini_umh); diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S index 9ea6100dca87..40311d10d2f2 100644 --- a/net/bpfilter/bpfilter_umh_blob.S +++ b/net/bpfilter/bpfilter_umh_blob.S @@ -1,5 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0 */ - .section .rodata, "a" + .section .init.rodata, "a" .global bpfilter_umh_start bpfilter_umh_start: .incbin "net/bpfilter/bpfilter_umh" diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c index 0480918bfc7c..9063c6767d34 100644 --- a/net/ipv4/bpfilter/sockopt.c +++ b/net/ipv4/bpfilter/sockopt.c @@ -12,15 +12,14 @@ struct bpfilter_umh_ops bpfilter_ops; EXPORT_SYMBOL_GPL(bpfilter_ops); -static void bpfilter_umh_cleanup(struct umh_info *info) +void bpfilter_umh_cleanup(struct umd_info *info) { - mutex_lock(&bpfilter_ops.lock); - bpfilter_ops.stop = true; fput(info->pipe_to_umh); fput(info->pipe_from_umh); - info->pid = 0; - mutex_unlock(&bpfilter_ops.lock); + put_pid(info->tgid); + info->tgid = NULL; } +EXPORT_SYMBOL_GPL(bpfilter_umh_cleanup); static int bpfilter_mbox_request(struct sock *sk, int optname, char __user *optval, @@ -38,7 +37,11 @@ static int bpfilter_mbox_request(struct sock *sk, int optname, goto out; } } - if (bpfilter_ops.stop) { + if (bpfilter_ops.info.tgid && + thread_group_exited(bpfilter_ops.info.tgid)) + bpfilter_umh_cleanup(&bpfilter_ops.info); + + if (!bpfilter_ops.info.tgid) { err = bpfilter_ops.start(); if (err) goto out; @@ -69,9 +72,8 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, char __user *optval, static int __init bpfilter_sockopt_init(void) { mutex_init(&bpfilter_ops.lock); - bpfilter_ops.stop = true; - bpfilter_ops.info.cmdline = "bpfilter_umh"; - bpfilter_ops.info.cleanup = &bpfilter_umh_cleanup; + bpfilter_ops.info.tgid = NULL; + bpfilter_ops.info.driver_name = "bpfilter_umh"; return 0; } |