| Message ID | 87y2o1swee.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make the user mode driver code a better citizen | expand |
On 2020/07/03 1:40, Eric W. Biederman wrote: > > 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 won't test this version, for you are ignoring my comments.
I have merged all of this into my exec-next tree.
The code is also available on the frozen branch:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git usermode-driver-cleanup
The range-diff from the last posted version is below.
I was asked "Is there a simpler version of this code that could
be used for backports?". The honest answer is not really.
Fundamentally do_execve_file as it existed prior to this set of changes
breaks a lot of invariants in exec. The choices are either track down
all of the invariants it violates and fix it, or reorganize the code so
that do_execve_file is unnecessary. Reorganizing the code was the path
I found simplest and most reliable. I don't think anyone has tracked
down all of the constraints the code violated.
There is an issue clearly pointed out by Tetsuo Handa that in theory if
there is too long of a delay between closing the file after writing it
and flush_delayed_fput might not synchronize the file synchronously. I
can not trigger it, and this is the same code path the initramfs relies
upon. So I think calling flush_delayed_fput is good enough for this set
of changes.
If and when a generally accepted way to remove the theoreticaly race
it will be trivial to fix flush_delayed_fput or replace it and none
of the other logic changes.
Declaring this set of changes done now, allows the work that depends
upon this change to proceed.
Eric
---
1: 8fee10be3e7e ! 1: 5fec25f2cb95 umh: Capture the pid in umh_pipe_setup
@@ Commit message
v1: https://lkml.kernel.org/r/87h7uygf9i.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/875zb97iix.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-1-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/umh.h ##
2: 2d97bc5269dd ! 2: b044fa2ae50d umh: Move setting PF_UMH into umh_pipe_setup
@@ Commit message
v1: https://lkml.kernel.org/r/87bll6gf8t.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87zh8l63xs.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-2-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## kernel/umh.c ##
3: 974e2b827aca ! 3: 3a171042aeab umh: Rename the user mode driver helpers for clarity
@@ Commit message
v1: https://lkml.kernel.org/r/875zbegf82.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87tuyt63x3.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-3-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## kernel/umh.c ##
4: 6c8f72f8eb49 ! 4: 21d598280675 umh: Remove call_usermodehelper_setup_file.
@@ Commit message
v1: https://lkml.kernel.org/r/87zh8qf0mp.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87o8p163u1.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-4-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/umh.h ##
5: cbf6c2b5a04a ! 5: 884c5e683b67 umh: Separate the user mode driver and the user mode helper support
@@ Commit message
v1: https://lkml.kernel.org/r/87tuyyf0ln.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87imf963s6.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-5-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/bpfilter.h ##
6: b68617fd4ee3 ! 6: 74be2d3b80af umd: For clarity rename umh_info umd_info
@@ Commit message
v1: https://lkml.kernel.org/r/87o8p6f0kw.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/878sg563po.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-6-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/bpfilter.h ##
7: 6881acff5f6a ! 7: 1199c6c3da51 umd: Rename umd_info.cmdline umd_info.driver_name
@@ Commit message
v1: https://lkml.kernel.org/r/87imfef0k3.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87366d63os.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-7-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/usermode_driver.h ##
8: cd210622ff6f ! 8: e2dc9bf3f527 umd: Transform fork_usermode_blob into fork_usermode_driver
@@ Commit message
[1] https://lore.kernel.org/linux-fsdevel/2a8775b4-1dd5-9d5c-aa42-9872445e0942@i-love.sakura.ne.jp/
v1: https://lkml.kernel.org/r/87d05mf0j9.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87wo3p4p35.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-8-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/usermode_driver.h ##
9: 74d65aaf2cab ! 9: 55e6074e3fa6 umh: Stop calling do_execve_file
@@ Commit message
v1: https://lkml.kernel.org/r/877dvuf0i7.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87r1tx4p2a.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-9-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/umh.h ##
10: 58a9854274a1 ! 10: 25cf336de51b exec: Remove do_execve_file
@@ Commit message
[1] https://lore.kernel.org/linux-fsdevel/2a8775b4-1dd5-9d5c-aa42-9872445e0942@i-love.sakura.ne.jp/
v1: https://lkml.kernel.org/r/871rm2f0hi.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87lfk54p0m.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-10-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/exec.c ##
11: c45ae16a18c9 ! 11: 0fe3c63148ef bpfilter: Move bpfilter_umh back into init data
@@ Commit message
v1: https://lkml.kernel.org/r/87sgeidlvq.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87ftad4ozc.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-11-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## net/bpfilter/bpfilter_umh_blob.S ##
12: 43b41b9d52a0 ! 12: 1c340ead18ee umd: Track user space drivers with struct pid
@@ Commit message
v1: https://lkml.kernel.org/r/87mu4qdlv2.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/a70l4oy8.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-12-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/usermode_driver.h ##
13: 653476c24a30 ! 13: 38fd525a4c61 exit: Factor thread_group_exited out of pidfd_poll
@@ Metadata
## Commit message ##
exit: Factor thread_group_exited out of pidfd_poll
- Create an independent helper thread_group_exited report return true
+ Create an independent helper thread_group_exited which returns true
when all threads have passed exit_notify in do_exit. AKA all of the
threads are at least zombies and might be dead or completely gone.
- Create this helper by taking the logic out of pidfd_poll where
- it is already tested, and adding a missing READ_ONCE on
- the read of task->exit_state.
+ Create this helper by taking the logic out of pidfd_poll where it is
+ already tested, and adding a READ_ONCE on the read of
+ task->exit_state.
I will be changing the user mode driver code to use this same logic
to know when a user mode driver needs to be restarted.
@@ Commit message
Place the new helper thread_group_exited in kernel/exit.c and
EXPORT it so it can be used by modules.
+ Link: https://lkml.kernel.org/r/20200702164140.4468-13-ebiederm@xmission.com
+ Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/sched/signal.h ##
@@ kernel/exit.c: COMPAT_SYSCALL_DEFINE5(waitid,
+ * thread_group_exited - check that a thread group has exited
+ * @pid: tgid of thread group to be checked.
+ *
-+ * Test if thread group is has exited (all threads are zombies, dead
-+ * or completely gone).
++ * Test if the thread group represented by tgid has exited (all
++ * threads are zombies, dead or completely gone).
+ *
+ * Return: true if the thread group has exited. false otherwise.
+ */
14: 7ad037d12723 ! 14: e80eb1dc868b bpfilter: Take advantage of the facilities of struct pid
@@ Commit message
v1: https://lkml.kernel.org/r/87h7uydlu9.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/874kqt4owu.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-14-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/bpfilter.h ##
15: e50cf5e57a62 ! 15: 8c2f52663973 umd: Remove exit_umh
@@ Commit message
v1: https://lkml.kernel.org/r/87bll6dlte.fsf_-_@x220.int.ebiederm.org
v2: https://lkml.kernel.org/r/87y2o53abg.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-15-ebiederm@xmission.com
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## include/linux/sched.h ##
16: 32e057d8aa4a ! 16: 33c326014fe6 umd: Stop using split_argv
@@ Commit message
call_usermodehelper_setup.
v1: https://lkml.kernel.org/r/87sged3a9n.fsf_-_@x220.int.ebiederm.org
+ Link: https://lkml.kernel.org/r/20200702164140.4468-16-ebiederm@xmission.com
+ Acked-by: Alexei Starovoitov <ast@kernel.org>
+ Tested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## kernel/usermode_driver.c ##
On Thu, Jul 09, 2020 at 05:05:09PM -0500, Eric W. Biederman wrote: > > I have merged all of this into my exec-next tree. > > The code is also available on the frozen branch: > > git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git usermode-driver-cleanup > > Declaring this set of changes done now, allows the work that depends > upon this change to proceed. Now I've pulled it into bpf-next as well. In the mean time there were changes to kernel_write that broke bpfilter.ko I fixed it up as well. Thanks.
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 fs/exec.c | 38 ++------ include/linux/binfmts.h | 1 - include/linux/bpfilter.h | 7 +- include/linux/sched.h | 9 -- include/linux/sched/signal.h | 2 + include/linux/umh.h | 15 ---- include/linux/usermode_driver.h | 18 ++++ kernel/Makefile | 1 + kernel/exit.c | 25 +++++- kernel/fork.c | 6 +- kernel/umh.c | 171 +----------------------------------- kernel/usermode_driver.c | 182 +++++++++++++++++++++++++++++++++++++++ net/bpfilter/bpfilter_kern.c | 38 ++++---- net/bpfilter/bpfilter_umh_blob.S | 2 +- net/ipv4/bpfilter/sockopt.c | 20 +++-- 15 files changed, 275 insertions(+), 260 deletions(-) Eric W. Biederman (15): 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 bpfilter: Take advantage of the facilities of struct pid umd: Remove exit_umh umd: Stop using split_argv fs/exec.c | 38 ++------ include/linux/binfmts.h | 1 - include/linux/bpfilter.h | 7 +- include/linux/sched.h | 9 -- include/linux/umd.h | 18 ++++ include/linux/umh.h | 15 ---- kernel/Makefile | 1 + kernel/exit.c | 1 - kernel/umd.c | 182 +++++++++++++++++++++++++++++++++++++++ kernel/umh.c | 171 +----------------------------------- net/bpfilter/bpfilter_kern.c | 38 ++++---- net/bpfilter/bpfilter_umh_blob.S | 2 +- net/ipv4/bpfilter/sockopt.c | 20 +++-- 13 files changed, 248 insertions(+), 255 deletions(-)