[00/14] Make the user mode driver code a better citizen

Message

Eric W. Biederman June 26, 2020, 12:51 p.m. UTC

Asking for people to fix their bugs in this user mode driver code has
been remarkably unproductive.  So here are my bug fixes.

I have tested them by booting with the code compiled in and
by killing "bpfilter_umh" and running iptables -vnL to restart
the userspace driver.

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.

Please let me know if you see any bugs.  Once the code review is
finished I plan to take this through my tree.

Eric W. Biederman (14):
      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

 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                     | 183 +++++++++++++++++++++++++++++++++++++++
 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, 249 insertions(+), 255 deletions(-)

Eric

Comments

Eric W. Biederman June 26, 2020, 1:48 p.m. UTC | #1

Adding Luis Chamberlain as he maintains the user mode helper code.

Just so everyone who is relevant is at least aware of what is going on.

[e-mail] (Eric W. Biederman) writes:

> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive.  So here are my bug fixes.
>
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
>
> 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.
>
> Please let me know if you see any bugs.  Once the code review is
> finished I plan to take this through my tree.
>
> Eric W. Biederman (14):
>       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
>
>  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                     | 183 +++++++++++++++++++++++++++++++++++++++
>  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, 249 insertions(+), 255 deletions(-)
>
> Eric

Greg KH June 26, 2020, 2:10 p.m. UTC | #2

On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
> 
> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive.  So here are my bug fixes.
> 
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
> 
> 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.
> 
> Please let me know if you see any bugs.  Once the code review is
> finished I plan to take this through my tree.
> 
> Eric W. Biederman (14):
>       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

After a quick read, all looks sane to me, nice cleanups!

Reviewed-by: Greg Kroah-Hartman <[e-mail]>

Alexei Starovoitov June 26, 2020, 4:40 p.m. UTC | #3

On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
> 
> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive.  So here are my bug fixes.
> 
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
> 
> 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.
> 
> Please let me know if you see any bugs.  Once the code review is
> finished I plan to take this through my tree.

I did a quick look and I like the cleanup. Thanks! The end result looks good.
The only problem that you keep breaking the build between patches,
so series will not be bisectable.
blob_to_mnt is a great idea. Much better than embedding fs you advocated earlier.

I'm swamped with other stuff today and will test the set Sunday/Monday
with other patches that I'm working on.
I'm not sure why you want to rename the interface. Seems pointless. But fine.

As far as routing trees. Do you mind I'll take it via bpf-next ?
As I said countless times we're working on bpf_iter using fork_blob.
If you take this set via your tree we would need to wait the whole kernel release.
Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
I'd really like to avoid this huge delay.
Unless you can land it into 5.8-rc2 or rc3.

Eric W. Biederman June 26, 2020, 5:17 p.m. UTC | #4

Alexei Starovoitov <[e-mail]> writes:

> On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
>> 
>> Asking for people to fix their bugs in this user mode driver code has
>> been remarkably unproductive.  So here are my bug fixes.
>> 
>> I have tested them by booting with the code compiled in and
>> by killing "bpfilter_umh" and running iptables -vnL to restart
>> the userspace driver.
>> 
>> 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.
>> 
>> Please let me know if you see any bugs.  Once the code review is
>> finished I plan to take this through my tree.
>
> I did a quick look and I like the cleanup. Thanks!

Good then we have a path forward.

> The end result looks good.
> The only problem that you keep breaking the build between patches,
> so series will not be bisectable.

Keep breaking?  There is an issue with patch 5/14 where the build breaks
when bpfilter is not enabled.  Do you see any others? I know I tested
each patch individually.  But I was only testing with CONFIG_BPFILTER
enabled so I missed one.

So there should not be things that break
but things slip through occassionally.

I will resend this shortly with the fix and any others that I can find.

> blob_to_mnt is a great idea. Much better than embedding fs you advocated earlier.

I was lazy and not overdesigning but I still suspect the blob will
benefit from becoming a cpio in the future.

> I'm swamped with other stuff today and will test the set Sunday/Monday
> with other patches that I'm working on.
> I'm not sure why you want to rename the interface. Seems
> pointless. But fine.

For maintainability I think the code very much benefits from a clear
separation between the user mode driver code from the user mode helper
code.

> As far as routing trees. Do you mind I'll take it via bpf-next ?
> As I said countless times we're working on bpf_iter using fork_blob.
> If you take this set via your tree we would need to wait the whole kernel release.
> Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
> I'd really like to avoid this huge delay.
> Unless you can land it into 5.8-rc2 or rc3.

I also want to build upon this code.

How about when the review is done I post a frozen branch based on
v5.8-rc1 that you can merge into the bpf-next tree, and I can merge into
my branch.  That way we both can build upon this code.  That is the way
conflicts like this are usually handled.

Further I will leave any further enhancements to the user mode driver
infrastructure that people have suggested to you.

I will probably replace do_execve with a kernel_execve that doesn't need
set_fs() to copy the command line argument.  I haven't seen Christoph
Hellwig address that yet, and it looks pretty straight foward at this
point.


Eric

Alexei Starovoitov June 26, 2020, 6:22 p.m. UTC | #5

On Fri, Jun 26, 2020 at 12:17:40PM -0500, Eric W. Biederman wrote:
> 
> > I'm swamped with other stuff today and will test the set Sunday/Monday
> > with other patches that I'm working on.
> > I'm not sure why you want to rename the interface. Seems
> > pointless. But fine.
> 
> For maintainability I think the code very much benefits from a clear
> separation between the user mode driver code from the user mode helper
> code.

you mean different name gives that separation? makes sense.

> > As far as routing trees. Do you mind I'll take it via bpf-next ?
> > As I said countless times we're working on bpf_iter using fork_blob.
> > If you take this set via your tree we would need to wait the whole kernel release.
> > Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
> > I'd really like to avoid this huge delay.
> > Unless you can land it into 5.8-rc2 or rc3.
> 
> I also want to build upon this code.
> 
> How about when the review is done I post a frozen branch based on
> v5.8-rc1 that you can merge into the bpf-next tree, and I can merge into
> my branch.  That way we both can build upon this code.  That is the way
> conflicts like this are usually handled.

sure. that works too.

> Further I will leave any further enhancements to the user mode driver
> infrastructure that people have suggested to you.

ok

Tetsuo Handa June 27, 2020, 11:38 a.m. UTC | #6

On 2020/06/26 21:51, Eric W. Biederman wrote:
> Please let me know if you see any bugs.  Once the code review is
> finished I plan to take this through my tree.

This series needs some sanity checks.

diff --git a/kernel/umd.c b/kernel/umd.c
index de2f542191e5..f3e0227a3012 100644
--- a/kernel/umd.c
+++ b/kernel/umd.c
@@ -47,15 +47,18 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 
 /**
  * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
- * @info: information about usermode driver
- * @data: a blob of bytes that can be executed as a file
- * @len:  The lentgh of the blob
+ * @info: information about usermode driver (shouldn't be NULL)
+ * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
+ * @len:  The lentgh of the blob (shouldn't be 0)
  *
  */
 int umd_load_blob(struct umd_info *info, const void *data, size_t len)
 {
 	struct vfsmount *mnt;
 
+	if (!info || !info->driver_name || !data || !len)
+		return -EINVAL;
+
 	if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
 		return -EBUSY;
 
@@ -158,6 +161,9 @@ int fork_usermode_driver(struct umd_info *info)
 	char **argv = NULL;
 	int err;
 
+	if (!info || !info->driver_name)
+		return -EINVAL;
+
 	if (WARN_ON_ONCE(info->tgid))
 		return -EBUSY;
 
But loading

----- test.c -----
#include <linux/slab.h>
#include <linux/module.h>
#include <linux/umd.h>

static int __init test_init(void)
{
	const char blob[464] = {
		"\x7f\x45\x4c\x46\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x02\x00\x3e\x00\x01\x00\x00\x00\x80\x00\x40\x00\x00\x00\x00\x00"
		"\x40\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x40\x00\x38\x00\x01\x00\x40\x00\x04\x00\x03\x00"
		"\x01\x00\x00\x00\x05\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x40\x00\x00\x00\x00\x00\x00\x00\x40\x00\x00\x00\x00\x00"
		"\xb4\x00\x00\x00\x00\x00\x00\x00\xb4\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x20\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\xb8\x01\x00\x00\x00\xbf\x01\x00\x00\x00\x48\xbe\xa8\x00\x40\x00"
		"\x00\x00\x00\x00\xba\x0c\x00\x00\x00\x0f\x05\xb8\xe7\x00\x00\x00"
		"\xbf\x00\x00\x00\x00\x0f\x05\x00\x48\x65\x6c\x6c\x6f\x20\x77\x6f"
		"\x72\x6c\x64\x0a\x00\x2e\x73\x68\x73\x74\x72\x74\x61\x62\x00\x2e"
		"\x74\x65\x78\x74\x00\x2e\x72\x6f\x64\x61\x74\x61\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x0b\x00\x00\x00\x01\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00\x00"
		"\x80\x00\x40\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00"
		"\x27\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x10\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x11\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00"
		"\xa8\x00\x40\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00"
		"\x0c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x01\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x00\x00\x00\x00\x00\x00\x00\x00\xb4\x00\x00\x00\x00\x00\x00\x00"
		"\x19\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
		"\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
	};
	struct umd_info *info = kzalloc(sizeof(*info), GFP_KERNEL);
	
	if (!info)
		return -ENOMEM;
	info->driver_name = kstrdup("my test driver", GFP_KERNEL);
	printk("umd_load_blob()=%d\n", umd_load_blob(info, blob, 464));
	//printk("fork_usermode_driver()=%d\n", fork_usermode_driver(info));
	return -EINVAL;
}

module_init(test_init);
MODULE_LICENSE("GPL");
----- test.c -----

causes

   BUG_ON(!(task->flags & PF_KTHREAD));

in __fput_sync(). Do we want to forbid umd_load_blob() from process context (e.g.
upon module initialization time) ?

Also, since umd_load_blob() uses info->driver_name as filename, info->driver_name has to
satisfy strchr(info->driver_name, '/') == NULL && strlen(info->driver_name) <= NAME_MAX
in order to avoid -ENOENT failure. On the other hand, since fork_usermode_driver() uses
info->driver_name as argv[], info->driver_name has to use ' ' within this constraint.
This might be inconvenient...

Eric W. Biederman June 27, 2020, 12:59 p.m. UTC | #7

Tetsuo Handa <[e-mail]> writes:

> On 2020/06/26 21:51, Eric W. Biederman wrote:
>> Please let me know if you see any bugs.  Once the code review is
>> finished I plan to take this through my tree.
>

[sniped example code]
> causes
>
>    BUG_ON(!(task->flags & PF_KTHREAD));
>
> in __fput_sync(). Do we want to forbid umd_load_blob() from process context (e.g.
> upon module initialization time) ?

Interesting.  I had not realized that fput_sync would not work from
module context.

Forcing the fput to finish is absolutely necessary.  Otherwise the file
will still be open for write and deny_write_access in execve will fail.

Can you try replacing the __fput_sync with:
	fput(file);
        flush_delayed_fput();
        task_work_run();


Given that there is a big requirement for the code to run before init
I don't necessarily think it is a problem __fput_sync is a problem.
But it also seems silly to forbid modules if we can easily fix
the code.

Eric

Tetsuo Handa June 27, 2020, 1:57 p.m. UTC | #8

On 2020/06/27 21:59, Eric W. Biederman wrote:
> Can you try replacing the __fput_sync with:
> 	fput(file);
>         flush_delayed_fput();
>         task_work_run();

With below change, TOMOYO can obtain pathname like "tmpfs:/my\040test\040driver".

Please avoid WARN_ON() if printk() is sufficient (for friendliness to panic_on_warn=1 environments).
For argv[], I guess that fork_usermode_driver() should receive argv[] as argument rather than
trying to split info->driver_name, for somebody might want to pass meaningful argv[] (and
TOMOYO wants to use meaningful argv[] as a hint for identifying the intent).

diff --git a/kernel/umd.c b/kernel/umd.c
index de2f542191e5..ae6e85283f13 100644
--- a/kernel/umd.c
+++ b/kernel/umd.c
@@ -7,6 +7,7 @@
 #include <linux/mount.h>
 #include <linux/fs_struct.h>
 #include <linux/umd.h>
+#include <linux/task_work.h>
 
 static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *name)
 {
@@ -25,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 	if (IS_ERR(mnt))
 		return mnt;
 
-	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
+	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
 	if (IS_ERR(file)) {
 		mntput(mnt);
 		return ERR_CAST(file);
@@ -41,23 +42,33 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 		return ERR_PTR(err);
 	}
 
-	__fput_sync(file);
+	if (current->flags & PF_KTHREAD) {
+		__fput_sync(file);
+	} else {
+		fput(file);
+		flush_delayed_fput();
+		task_work_run();
+	}
 	return mnt;
 }
 
 /**
  * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
- * @info: information about usermode driver
- * @data: a blob of bytes that can be executed as a file
- * @len:  The lentgh of the blob
+ * @info: information about usermode driver (shouldn't be NULL)
+ * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
+ * @len:  The lentgh of the blob (shouldn't be 0)
  *
  */
 int umd_load_blob(struct umd_info *info, const void *data, size_t len)
 {
 	struct vfsmount *mnt;
 
-	if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
+	if (!info || !info->driver_name || !data || !len)
+		return -EINVAL;
+	if (info->wd.dentry || info->wd.mnt) {
+		pr_info("%s already loaded.\n", info->driver_name);
 		return -EBUSY;
+	}
 
 	mnt = blob_to_mnt(data, len, info->driver_name);
 	if (IS_ERR(mnt))
@@ -71,14 +82,14 @@ EXPORT_SYMBOL_GPL(umd_load_blob);
 
 /**
  * umd_unload_blob - Disassociate @info from a previously loaded blob
- * @info: information about usermode driver
+ * @info: information about usermode driver (shouldn't be NULL)
  *
  */
 int umd_unload_blob(struct umd_info *info)
 {
-	if (WARN_ON_ONCE(!info->wd.mnt ||
-			 !info->wd.dentry ||
-			 info->wd.mnt->mnt_root != info->wd.dentry))
+	if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
+		return -EINVAL;
+	if (WARN_ON_ONCE(info->wd.mnt->mnt_root != info->wd.dentry))
 		return -EINVAL;
 
 	kern_unmount(info->wd.mnt);
@@ -158,8 +169,14 @@ int fork_usermode_driver(struct umd_info *info)
 	char **argv = NULL;
 	int err;
 
-	if (WARN_ON_ONCE(info->tgid))
+	if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
+		return -EINVAL;
+	if (WARN_ON_ONCE(info->wd.mnt->mnt_root != info->wd.dentry))
+		return -EINVAL;
+	if (info->tgid) {
+		pr_info("%s already running.\n", info->driver_name);
 		return -EBUSY;
+	}
 
 	err = -ENOMEM;
 	argv = argv_split(GFP_KERNEL, info->driver_name, NULL);

Alexei Starovoitov June 28, 2020, 7:44 p.m. UTC | #9

On Sat, Jun 27, 2020 at 10:57:10PM +0900, Tetsuo Handa wrote:
> On 2020/06/27 21:59, Eric W. Biederman wrote:
> > Can you try replacing the __fput_sync with:
> > 	fput(file);
> >         flush_delayed_fput();
> >         task_work_run();
> 
> With below change, TOMOYO can obtain pathname like "tmpfs:/my\040test\040driver".
> 
> Please avoid WARN_ON() if printk() is sufficient (for friendliness to panic_on_warn=1 environments).
> For argv[], I guess that fork_usermode_driver() should receive argv[] as argument rather than
> trying to split info->driver_name, for somebody might want to pass meaningful argv[] (and
> TOMOYO wants to use meaningful argv[] as a hint for identifying the intent).
> 
> diff --git a/kernel/umd.c b/kernel/umd.c
> index de2f542191e5..ae6e85283f13 100644
> --- a/kernel/umd.c
> +++ b/kernel/umd.c
> @@ -7,6 +7,7 @@
>  #include <linux/mount.h>
>  #include <linux/fs_struct.h>
>  #include <linux/umd.h>
> +#include <linux/task_work.h>
>  
>  static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *name)
>  {
> @@ -25,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
>  	if (IS_ERR(mnt))
>  		return mnt;
>  
> -	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
> +	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
>  	if (IS_ERR(file)) {
>  		mntput(mnt);
>  		return ERR_CAST(file);
> @@ -41,23 +42,33 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
>  		return ERR_PTR(err);
>  	}
>  
> -	__fput_sync(file);
> +	if (current->flags & PF_KTHREAD) {
> +		__fput_sync(file);
> +	} else {
> +		fput(file);
> +		flush_delayed_fput();
> +		task_work_run();
> +	}

Thanks. This makes sense to me.

>  	return mnt;
>  }
>  
>  /**
>   * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
> - * @info: information about usermode driver
> - * @data: a blob of bytes that can be executed as a file
> - * @len:  The lentgh of the blob
> + * @info: information about usermode driver (shouldn't be NULL)
> + * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
> + * @len:  The lentgh of the blob (shouldn't be 0)
>   *
>   */
>  int umd_load_blob(struct umd_info *info, const void *data, size_t len)
>  {
>  	struct vfsmount *mnt;
>  
> -	if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
> +	if (!info || !info->driver_name || !data || !len)
> +		return -EINVAL;
> +	if (info->wd.dentry || info->wd.mnt) {
> +		pr_info("%s already loaded.\n", info->driver_name);
>  		return -EBUSY;
> +	}

But all the defensive programming kinda goes against general kernel style.
I wouldn't do it. Especially pr_info() ?!
Though I don't feel strongly about it.

I would like to generalize elf_header_check() a bit and call it
before doing blob_to_mnt() to make sure that all blobs are elf files only.
Supporting '#!/bin/bash' or other things as blobs seems wrong to me.

Tetsuo Handa June 29, 2020, 2:20 a.m. UTC | #10

On 2020/06/29 4:44, Alexei Starovoitov wrote:
> But all the defensive programming kinda goes against general kernel style.
> I wouldn't do it. Especially pr_info() ?!
> Though I don't feel strongly about it.

Honestly speaking, caller should check for errors and print appropriate
messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
went wrong (maybe memory corruption). But other conditions are not fatal.
That is, I consider even pr_info() here should be unnecessary.

> 
> I would like to generalize elf_header_check() a bit and call it
> before doing blob_to_mnt() to make sure that all blobs are elf files only.
> Supporting '#!/bin/bash' or other things as blobs seems wrong to me.

Why? There is no point with forbidding "#!", for users can use a wrapper
ELF binary which contains instructions including glibc's execv()/system()
functions even if "#!" cannot be used.

What is more important is what protection/isolation properties processes started
via fork_usermode_driver() should hold, for ELF binary can contain arbitrary
instructions, these processes run as daemons (reading request from stdin and
writing response to stdout) but hidden from "/usr/bin/pstree -p 1" (because
they are forked from kthreadd kernel thread).

Eric W. Biederman June 29, 2020, 7:55 p.m. UTC | #11

This is the second 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.

I have compiled tested each change with and without CONFIG_BPFILTER
enabled.

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.

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(-)

v1: https://lkml.kernel.org/r/[e-mail]
---
git range-diff master v1 v2

 1:  2b76f9b3158d !  1:  d8fb851fa3d8 umh: Capture the pid in umh_pipe_setup
    @@ Commit message
         code that is specific to user mode drivers from the common user path of
         user mode helpers.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umh.h ##
 2:  d853e933ae32 !  2:  b191c5df43ec umh: Move setting PF_UMH into umh_pipe_setup
    @@ Commit message
         Setting PF_UMH unconditionally is harmless as an action will only
         happen if it is paired with an entry on umh_list.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## kernel/umh.c ##
 3:  92d2550f0d6a !  3:  74e8c0bf3076 umh: Rename the user mode driver helpers for clarity
    @@ Commit message
         don't make much sense.  Instead name them  umd_setup and umd_cleanup
         for the functional role in setting up user mode drivers.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## kernel/umh.c ##
    @@ kernel/umh.c: static int umh_pipe_setup(struct subprocess_info *info, struct cre
      {
      	struct umh_info *umh_info = info->data;
      
    +-	/* cleanup if umh_pipe_setup() was successful but exec failed */
    ++	/* cleanup if umh_setup() was successful but exec failed */
    + 	if (info->retval) {
    + 		fput(umh_info->pipe_to_umh);
    + 		fput(umh_info->pipe_from_umh);
     @@ kernel/umh.c: int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
      	}
      
 4:  5a9cc2c6c64f !  4:  6652f7c0a909 umh: Remove call_usermodehelper_setup_file.
    @@ Commit message
         For this to work the argv_free is moved from umh_clean_and_save_pid
         to fork_usermode_blob.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umh.h ##
 5:  03ed13fa8eee !  5:  2a1ccb05cf9f umh: Separate the user mode driver and the user mode helper support
    @@ Commit message
         This makes the kernel smaller for everyone who does not use a usermode
         driver.
     
    +    v2: Moved exit_umh from sched.h to umd.h and handle the case when the
    +    code is compiled out.
    +
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/bpfilter.h ##
    @@ include/linux/bpfilter.h
      struct sock;
      int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char __user *optval,
     
    + ## include/linux/sched.h ##
    +@@ include/linux/sched.h: static inline void rseq_execve(struct task_struct *t)
    + 
    + #endif
    + 
    +-void __exit_umh(struct task_struct *tsk);
    +-
    +-static inline void exit_umh(struct task_struct *tsk)
    +-{
    +-	if (unlikely(tsk->flags & PF_UMH))
    +-		__exit_umh(tsk);
    +-}
    +-
    + #ifdef CONFIG_DEBUG_RSEQ
    + 
    + void rseq_syscall(struct pt_regs *regs);
    +
      ## include/linux/umd.h (new) ##
     @@
     +#ifndef __LINUX_UMD_H__
    @@ include/linux/umd.h (new)
     +
     +#include <linux/umh.h>
     +
    ++#ifdef CONFIG_BPFILTER
    ++void __exit_umh(struct task_struct *tsk);
    ++
    ++static inline void exit_umh(struct task_struct *tsk)
    ++{
    ++	if (unlikely(tsk->flags & PF_UMH))
    ++		__exit_umh(tsk);
    ++}
    ++#else
    ++static inline void exit_umh(struct task_struct *tsk)
    ++{
    ++}
    ++#endif
    ++
     +struct umh_info {
     +	const char *cmdline;
     +	struct file *pipe_to_umh;
    @@ kernel/Makefile: obj-y     = fork.o exec_domain.o panic.o \
      obj-$(CONFIG_MULTIUSER) += groups.o
      
     
    + ## kernel/exit.c ##
    +@@
    + #include <linux/random.h>
    + #include <linux/rcuwait.h>
    + #include <linux/compat.h>
    ++#include <linux/umd.h>
    + 
    + #include <linux/uaccess.h>
    + #include <asm/unistd.h>
    +
      ## kernel/umd.c (new) ##
     @@
     +// SPDX-License-Identifier: GPL-2.0-only
    @@ kernel/umd.c (new)
     +{
     +	struct umh_info *umh_info = info->data;
     +
    -+	/* cleanup if umh_pipe_setup() was successful but exec failed */
    ++	/* cleanup if umh_setup() was successful but exec failed */
     +	if (info->retval) {
     +		fput(umh_info->pipe_to_umh);
     +		fput(umh_info->pipe_from_umh);
    @@ kernel/umh.c: struct subprocess_info *call_usermodehelper_setup(const char *path
     -{
     -	struct umh_info *umh_info = info->data;
     -
    --	/* cleanup if umh_pipe_setup() was successful but exec failed */
    +-	/* cleanup if umh_setup() was successful but exec failed */
     -	if (info->retval) {
     -		fput(umh_info->pipe_to_umh);
     -		fput(umh_info->pipe_from_umh);
 6:  698bfbcb6c7f !  6:  b16081fb8d92 umd: For clarity rename umh_info umd_info
    @@ Commit message
         This structure is only used for user mode drivers so change
         the prefix from umh to umd to make that clear.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/bpfilter.h ##
    @@ include/linux/bpfilter.h: int bpfilter_ip_set_sockopt(struct sock *sk, int optna
      	int (*sockopt)(struct sock *sk, int optname,
     
      ## include/linux/umd.h ##
    -@@
    - 
    - #include <linux/umh.h>
    +@@ include/linux/umd.h: static inline void exit_umh(struct task_struct *tsk)
    + }
    + #endif
      
     -struct umh_info {
     +struct umd_info {
    @@ kernel/umd.c: static int umd_setup(struct subprocess_info *info, struct cred *ne
     -	struct umh_info *umh_info = info->data;
     +	struct umd_info *umd_info = info->data;
      
    - 	/* cleanup if umh_pipe_setup() was successful but exec failed */
    + 	/* cleanup if umh_setup() was successful but exec failed */
      	if (info->retval) {
     -		fput(umh_info->pipe_to_umh);
     -		fput(umh_info->pipe_from_umh);
 7:  9cdcb5e7fc61 !  7:  42c13aa9c526 umd: Rename umd_info.cmdline umd_info.driver_name
    @@ Commit message
         driver_name any place where the code is looking for a name
         of the binary.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umd.h ##
    -@@
    - #include <linux/umh.h>
    +@@ include/linux/umd.h: static inline void exit_umh(struct task_struct *tsk)
    + #endif
      
      struct umd_info {
     -	const char *cmdline;
 8:  5ada2f70ae21 !  8:  385ed14a025b umd: Transform fork_usermode_blob into fork_usermode_driver
    @@ Commit message
         path based LSMs there are no new special cases.
     
         [1] https://lore.kernel.org/linux-fsdevel/[e-mail]/
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umd.h ##
    @@ include/linux/umd.h
      #include <linux/umh.h>
     +#include <linux/path.h>
      
    - struct umd_info {
    - 	const char *driver_name;
    + #ifdef CONFIG_BPFILTER
    + void __exit_umh(struct task_struct *tsk);
     @@ include/linux/umd.h: struct umd_info {
      	struct file *pipe_from_umh;
      	struct list_head list;
    @@ kernel/umd.c
      #include <linux/pipe_fs_i.h>
     +#include <linux/mount.h>
     +#include <linux/fs_struct.h>
    ++#include <linux/task_work.h>
      #include <linux/umd.h>
      
      static LIST_HEAD(umh_list);
    @@ kernel/umd.c
     +		return ERR_PTR(err);
     +	}
     +
    -+	__fput_sync(file);
    ++	fput(file);
    ++
    ++	/* Flush delayed fput so exec can open the file read-only */
    ++	flush_delayed_fput();
    ++	task_work_run();
     +	return mnt;
     +}
     +
 9:  e4ff478e77c9 !  9:  eeae92e3f0da umh: Stop calling do_execve_file
    @@ Commit message
         call_usermodehelper_exec_async that would call do_execve_file instead
         of do_execve if file was set.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umh.h ##
10:  dc0a38f6bd51 ! 10:  c7fdaf5660b8 exec: Remove do_execve_file
    @@ Commit message
     
         Reported-by: Tetsuo Handa <[e-mail]>
         [1] https://lore.kernel.org/linux-fsdevel/[e-mail]/
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## fs/exec.c ##
11:  d0c0c2ddf53b ! 11:  43d08e6986a7 bpfilter: Move bpfilter_umh back into init data
    @@ Commit message
         the blob the blob no longer needs to live .rodata to allow for restarting.
         So move the blob back to .init.rodata.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## net/bpfilter/bpfilter_umh_blob.S ##
12:  51b703ad75dd ! 12:  729ee744af46 umd: Track user space drivers with struct pid
    @@ Commit message
         As the tgid is now refcounted verify the tgid is NULL at the start of
         fork_usermode_driver to avoid the possibility of silent pid leaks.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/umd.h ##
13:  cdadf89503c9 ! 13:  2d85b10b965e bpfilter: Take advantage of the facilities of struct pid
    @@ Commit message
         struct pid can be tested to see if a process still exists, and that
         struct pid has a wait queue that notifies when the process dies.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/bpfilter.h ##
14:  1d621649e144 ! 14:  6e7e8ddd2b44 umd: Remove exit_umh
    @@ Commit message
         callback is what exit_umh exists to call.  So remove exit_umh and all
         of it's associated booking.
     
    +    Link: https://lkml.kernel.org/r/[e-mail]
    +    Reviewed-by: Greg Kroah-Hartman <[e-mail]>
         Signed-off-by: "Eric W. Biederman" <[e-mail]>
     
      ## include/linux/sched.h ##
    @@ include/linux/sched.h: extern struct pid *cad_pid;
      #define PF_NO_SETAFFINITY	0x04000000	/* Userland is not allowed to meddle with cpus_mask */
      #define PF_MCE_EARLY		0x08000000      /* Early kill for mce process policy */
      #define PF_MEMALLOC_NOCMA	0x10000000	/* All allocation request will have _GFP_MOVABLE cleared */
    -@@ include/linux/sched.h: static inline void rseq_execve(struct task_struct *t)
    - 
    - #endif
    +
    + ## include/linux/umd.h ##
    +@@
    + #include <linux/umh.h>
    + #include <linux/path.h>
      
    +-#ifdef CONFIG_BPFILTER
     -void __exit_umh(struct task_struct *tsk);
     -
     -static inline void exit_umh(struct task_struct *tsk)
    @@ include/linux/sched.h: static inline void rseq_execve(struct task_struct *t)
     -	if (unlikely(tsk->flags & PF_UMH))
     -		__exit_umh(tsk);
     -}
    +-#else
    +-static inline void exit_umh(struct task_struct *tsk)
    +-{
    +-}
    +-#endif
     -
    - #ifdef CONFIG_DEBUG_RSEQ
    - 
    - void rseq_syscall(struct pt_regs *regs);
    -
    - ## include/linux/umd.h ##
    -@@ include/linux/umd.h: struct umd_info {
    + struct umd_info {
      	const char *driver_name;
      	struct file *pipe_to_umh;
      	struct file *pipe_from_umh;
    @@ include/linux/umd.h: struct umd_info {
      };
     
      ## kernel/exit.c ##
    +@@
    + #include <linux/random.h>
    + #include <linux/rcuwait.h>
    + #include <linux/compat.h>
    +-#include <linux/umd.h>
    + 
    + #include <linux/uaccess.h>
    + #include <asm/unistd.h>
     @@ kernel/exit.c: void __noreturn do_exit(long code)
      	exit_task_namespaces(tsk);
      	exit_task_work(tsk);
    @@ kernel/exit.c: void __noreturn do_exit(long code)
     
      ## kernel/umd.c ##
     @@
    - #include <linux/fs_struct.h>
    + #include <linux/task_work.h>
      #include <linux/umd.h>
      
     -static LIST_HEAD(umh_list);
 -:  ------------ > 15:  662deff06d76 umd: Stop using split_argv

Eric W. Biederman June 29, 2020, 8:19 p.m. UTC | #12

Tetsuo Handa <[e-mail]> writes:

> On 2020/06/29 4:44, Alexei Starovoitov wrote:
>> But all the defensive programming kinda goes against general kernel style.
>> I wouldn't do it. Especially pr_info() ?!
>> Though I don't feel strongly about it.
>
> Honestly speaking, caller should check for errors and print appropriate
> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
> went wrong (maybe memory corruption). But other conditions are not fatal.
> That is, I consider even pr_info() here should be unnecessary.

They were all should never happen cases.  Which is why my patches do:
if (WARN_ON_ONCE(...))

That let's the caller know the messed up very clearly while still
providing a change to continue.

If they were clearly corruption no ones kernel should ever continue
BUG_ON would be appropriate.

>> I would like to generalize elf_header_check() a bit and call it
>> before doing blob_to_mnt() to make sure that all blobs are elf files only.
>> Supporting '#!/bin/bash' or other things as blobs seems wrong to me.

I vote for not worry about things that have never happened, and are
obviously incorrect.

The only points of checks like that is to catch cases where other
developers misunderstand the interface.  When you get to something like
sysfs with lots and lots of users where it is hard to audit there
is real value in sanity checks.  In something like this with very few
users. Just making the code clear should be enough for people not to do
ridiculous things.


In any case Tetsuo I will leave futher sanity checks for you and Alexei
to work out.  It is beyond the scope of my patchset, and they are easy
enough to add as follow on patches.

Eric

Alexei Starovoitov June 29, 2020, 10:12 p.m. UTC | #13

On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
> 
> 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.
> 
> I have compiled tested each change with and without CONFIG_BPFILTER
> enabled.

With
CONFIG_BPFILTER=y
CONFIG_BPFILTER_UMH=m
it doesn't build:

ERROR: modpost: "kill_pid_info" [net/bpfilter/bpfilter.ko] undefined!

I've added:
+EXPORT_SYMBOL(kill_pid_info);
to continue testing...

And then did:
while true; do iptables -L;rmmod bpfilter; done
 
Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().

I suspect patch 13 is somehow responsible:
+	if (tgid) {
+		kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
+		wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
+		bpfilter_umh_cleanup(info);
+	}

I cannot figure out why it hangs. Some sort of race ?
Since adding short delay between kill and wait makes it work.

Eric W. Biederman June 30, 2020, 1:13 a.m. UTC | #14

Alexei Starovoitov <[e-mail]> writes:

> On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
>> 
>> 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.
>> 
>> I have compiled tested each change with and without CONFIG_BPFILTER
>> enabled.
>
> With
> CONFIG_BPFILTER=y
> CONFIG_BPFILTER_UMH=m
> it doesn't build:
>
> ERROR: modpost: "kill_pid_info" [net/bpfilter/bpfilter.ko] undefined!
>
> I've added:
> +EXPORT_SYMBOL(kill_pid_info);
> to continue testing...
>
> And then did:
> while true; do iptables -L;rmmod bpfilter; done
>  
> Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().
>
> I suspect patch 13 is somehow responsible:
> +	if (tgid) {
> +		kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
> +		wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
> +		bpfilter_umh_cleanup(info);
> +	}
>
> I cannot figure out why it hangs. Some sort of race ?
> Since adding short delay between kill and wait makes it work.

Thanks.  I will take a look.

Eric

Tetsuo Handa June 30, 2020, 6:16 a.m. UTC | #15

On 2020/06/30 10:13, Eric W. Biederman wrote:
> Alexei Starovoitov <[e-mail]> writes:
> 
>> On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
>>>
>>> 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.
>>>
>>> I have compiled tested each change with and without CONFIG_BPFILTER
>>> enabled.
>>
>> With
>> CONFIG_BPFILTER=y
>> CONFIG_BPFILTER_UMH=m
>> it doesn't build:
>>
>> ERROR: modpost: "kill_pid_info" [net/bpfilter/bpfilter.ko] undefined!
>>
>> I've added:
>> +EXPORT_SYMBOL(kill_pid_info);
>> to continue testing...

kill_pid() is already exported.

>>
>> And then did:
>> while true; do iptables -L;rmmod bpfilter; done
>>  
>> Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().
>>
>> I suspect patch 13 is somehow responsible:
>> +	if (tgid) {
>> +		kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
>> +		wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
>> +		bpfilter_umh_cleanup(info);
>> +	}
>>
>> I cannot figure out why it hangs. Some sort of race ?
>> Since adding short delay between kill and wait makes it work.

Because there is a race window that detach_pid() from __unhash_process() from
__exit_signal() from release_task() from exit_notify() from do_exit() is called
some time after wake_up_all(&pid->wait_pidfd) from do_notify_pidfd() from
do_notify_parent() from exit_notify() from do_exit() was called (in other words,
we can't use pid->wait_pidfd when pid_task() is used at wait_event()) ?

Below are changes I suggest.

diff --git a/kernel/umd.c b/kernel/umd.c
index ff79fb16d738..f688813b8830 100644
--- a/kernel/umd.c
+++ b/kernel/umd.c
@@ -26,7 +26,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 	if (IS_ERR(mnt))
 		return mnt;
 
-	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY, 0700);
+	file = file_open_root(mnt->mnt_root, mnt, name, O_CREAT | O_WRONLY | O_EXCL, 0700);
 	if (IS_ERR(file)) {
 		mntput(mnt);
 		return ERR_CAST(file);
@@ -52,16 +52,18 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 
 /**
  * umd_load_blob - Remember a blob of bytes for fork_usermode_driver
- * @info: information about usermode driver
- * @data: a blob of bytes that can be executed as a file
- * @len:  The lentgh of the blob
+ * @info: information about usermode driver (shouldn't be NULL)
+ * @data: a blob of bytes that can be executed as a file (shouldn't be NULL)
+ * @len:  The lentgh of the blob (shouldn't be 0)
  *
  */
 int umd_load_blob(struct umd_info *info, const void *data, size_t len)
 {
 	struct vfsmount *mnt;
 
-	if (WARN_ON_ONCE(info->wd.dentry || info->wd.mnt))
+	if (!info || !info->driver_name || !data || !len)
+		return -EINVAL;
+	if (info->wd.dentry || info->wd.mnt)
 		return -EBUSY;
 
 	mnt = blob_to_mnt(data, len, info->driver_name);
@@ -76,15 +78,14 @@ EXPORT_SYMBOL_GPL(umd_load_blob);
 
 /**
  * umd_unload_blob - Disassociate @info from a previously loaded blob
- * @info: information about usermode driver
+ * @info: information about usermode driver (shouldn't be NULL)
  *
  */
 int umd_unload_blob(struct umd_info *info)
 {
-	if (WARN_ON_ONCE(!info->wd.mnt ||
-			 !info->wd.dentry ||
-			 info->wd.mnt->mnt_root != info->wd.dentry))
+	if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
 		return -EINVAL;
+	BUG_ON(info->wd.mnt->mnt_root != info->wd.dentry);
 
 	kern_unmount(info->wd.mnt);
 	info->wd.mnt = NULL;
@@ -138,7 +139,7 @@ static void umd_cleanup(struct subprocess_info *info)
 {
 	struct umd_info *umd_info = info->data;
 
-	/* cleanup if umh_setup() was successful but exec failed */
+	/* cleanup if umd_setup() was successful but exec failed */
 	if (info->retval) {
 		fput(umd_info->pipe_to_umh);
 		fput(umd_info->pipe_from_umh);
@@ -163,7 +164,10 @@ int fork_usermode_driver(struct umd_info *info)
 	const char *argv[] = { info->driver_name, NULL };
 	int err;
 
-	if (WARN_ON_ONCE(info->tgid))
+	if (!info || !info->driver_name || !info->wd.dentry || !info->wd.mnt)
+		return -EINVAL;
+	BUG_ON(info->wd.mnt->mnt_root != info->wd.dentry);
+	if (info->tgid)
 		return -EBUSY;
 
 	err = -ENOMEM;
diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index 91474884ddb7..9dd70aacb81a 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -19,8 +19,13 @@ static void shutdown_umh(void)
 	struct pid *tgid = info->tgid;
 
 	if (tgid) {
-		kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
-		wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
+		kill_pid(tgid, SIGKILL, 1);
+		while (({ bool done;
+			  rcu_read_lock();
+			  done = !pid_task(tgid, PIDTYPE_TGID);
+			  rcu_read_unlock();
+			  done; }))
+			schedule_timeout_uninterruptible(1);
 		bpfilter_umh_cleanup(info);
 	}
 }

Tetsuo Handa June 30, 2020, 6:28 a.m. UTC | #16

On 2020/06/30 5:19, Eric W. Biederman wrote:
> Tetsuo Handa <[e-mail]> writes:
> 
>> On 2020/06/29 4:44, Alexei Starovoitov wrote:
>>> But all the defensive programming kinda goes against general kernel style.
>>> I wouldn't do it. Especially pr_info() ?!
>>> Though I don't feel strongly about it.
>>
>> Honestly speaking, caller should check for errors and print appropriate
>> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
>> went wrong (maybe memory corruption). But other conditions are not fatal.
>> That is, I consider even pr_info() here should be unnecessary.
> 
> They were all should never happen cases.  Which is why my patches do:
> if (WARN_ON_ONCE(...))

No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
This bug was unfortunately not found by syzkaller because this path is
not easily reachable via syscall interface.

> 
> That let's the caller know the messed up very clearly while still
> providing a change to continue.
> 
> If they were clearly corruption no ones kernel should ever continue
> BUG_ON would be appropriate.

Please use BUG_ON() (to only corruption case) like I suggested in my updated diff.

Eric W. Biederman June 30, 2020, 12:29 p.m. UTC | #17

Alexei Starovoitov <[e-mail]> writes:

2> On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
>> 
>> 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.
>> 
>> I have compiled tested each change with and without CONFIG_BPFILTER
>> enabled.
>
> With
> CONFIG_BPFILTER=y
> CONFIG_BPFILTER_UMH=m
> it doesn't build:
>
> ERROR: modpost: "kill_pid_info" [net/bpfilter/bpfilter.ko] undefined!
>
> I've added:
> +EXPORT_SYMBOL(kill_pid_info);
> to continue testing...

I am rather surprised I thought Tetsuo had already compile tested
modules.


> I suspect patch 13 is somehow responsible:
> +	if (tgid) {
> +		kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
> +		wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
> +		bpfilter_umh_cleanup(info);
> +	}
>
> I cannot figure out why it hangs. Some sort of race ?
> Since adding short delay between kill and wait makes it work.

Having had a chance to sleep kill_pid_info was a thinko, as was
!pid_task.  It should have been !pid_has_task as that takes the proper
rcu locking.

I don't know if that is going to be enough to fix the wait_event
but those are obvious bugs that need to be fixed.

diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index 91474884ddb7..3e1874030daa 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -19,8 +19,8 @@ static void shutdown_umh(void)
        struct pid *tgid = info->tgid;
 
        if (tgid) {
-               kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
-               wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
+               kill_pid(tgid, SIGKILL, 1);
+               wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
                bpfilter_umh_cleanup(info);
        }
 }

> And then did:
> while true; do iptables -L;rmmod bpfilter; done
>  
> Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().

Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
release_task is called so there is a race.  As it is possible to wake
up and then go back to sleep before pid_has_task becomes false.

So I think I need a friendly helper that does:

bool task_has_exited(struct pid *tgid)
{
	bool exited = false;

	rcu_read_lock();
        tsk = pid_task(tgid, PIDTYPE_TGID);
        exited = !!tsk;
        if (tsk) {
        	exited = !!tsk->exit_state;
out:
	rcu_unlock();
	return exited;
}

There should be a sensible way to do that.

Eric

Eric W. Biederman June 30, 2020, 12:32 p.m. UTC | #18

Tetsuo Handa <[e-mail]> writes:

> On 2020/06/30 5:19, Eric W. Biederman wrote:
>> Tetsuo Handa <[e-mail]> writes:
>> 
>>> On 2020/06/29 4:44, Alexei Starovoitov wrote:
>>>> But all the defensive programming kinda goes against general kernel style.
>>>> I wouldn't do it. Especially pr_info() ?!
>>>> Though I don't feel strongly about it.
>>>
>>> Honestly speaking, caller should check for errors and print appropriate
>>> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
>>> went wrong (maybe memory corruption). But other conditions are not fatal.
>>> That is, I consider even pr_info() here should be unnecessary.
>> 
>> They were all should never happen cases.  Which is why my patches do:
>> if (WARN_ON_ONCE(...))
>
> No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
> This bug was unfortunately not found by syzkaller because this path is
> not easily reachable via syscall interface.

Absolutely yes.  These are cases that should never happen.
They should never be reachable by userspace.

It is absolutely a bug if these are hit by userspace.

Now if fuzzers want horrible cases to be even more horrible and change a
nice friendly warn into a panic that is their problem.  The issue being
do they capture the information the rest of us need to fix.

Eric

Tetsuo Handa June 30, 2020, 1:21 p.m. UTC | #19

On 2020/06/30 21:29, Eric W. Biederman wrote:
> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
> release_task is called so there is a race.  As it is possible to wake
> up and then go back to sleep before pid_has_task becomes false.

What is the reason we want to wait until pid_has_task() becomes false?

- wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
+ while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));




By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
that use of flush_delayed_fput() has to be careful. Al, is it safe to call
flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
called from both kernel thread and from process context (e.g. init_module()
syscall by /sbin/insmod )) ?

Alexei Starovoitov June 30, 2020, 4:48 p.m. UTC | #20

On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
> On 2020/06/30 5:19, Eric W. Biederman wrote:
> > Tetsuo Handa <[e-mail]> writes:
> > 
> >> On 2020/06/29 4:44, Alexei Starovoitov wrote:
> >>> But all the defensive programming kinda goes against general kernel style.
> >>> I wouldn't do it. Especially pr_info() ?!
> >>> Though I don't feel strongly about it.
> >>
> >> Honestly speaking, caller should check for errors and print appropriate
> >> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
> >> went wrong (maybe memory corruption). But other conditions are not fatal.
> >> That is, I consider even pr_info() here should be unnecessary.
> > 
> > They were all should never happen cases.  Which is why my patches do:
> > if (WARN_ON_ONCE(...))
> 
> No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.

I don't believe that's true.
Please show fuzzing stack trace to prove your point.

Alexei Starovoitov June 30, 2020, 4:52 p.m. UTC | #21

On Tue, Jun 30, 2020 at 07:29:34AM -0500, Eric W. Biederman wrote:
> 
> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
> index 91474884ddb7..3e1874030daa 100644
> --- a/net/bpfilter/bpfilter_kern.c
> +++ b/net/bpfilter/bpfilter_kern.c
> @@ -19,8 +19,8 @@ static void shutdown_umh(void)
>         struct pid *tgid = info->tgid;
>  
>         if (tgid) {
> -               kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
> -               wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
> +               kill_pid(tgid, SIGKILL, 1);
> +               wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>                 bpfilter_umh_cleanup(info);
>         }
>  }
> 
> > And then did:
> > while true; do iptables -L;rmmod bpfilter; done
> >  
> > Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().
> 
> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
> release_task is called so there is a race.  As it is possible to wake
> up and then go back to sleep before pid_has_task becomes false.
> 
> So I think I need a friendly helper that does:
> 
> bool task_has_exited(struct pid *tgid)
> {
> 	bool exited = false;
> 
> 	rcu_read_lock();
>         tsk = pid_task(tgid, PIDTYPE_TGID);
>         exited = !!tsk;
>         if (tsk) {
>         	exited = !!tsk->exit_state;
> out:
> 	rcu_unlock();
> 	return exited;
> }

All makes sense to me.
If I understood the race condition such helper should indeed solve it.
Are you going to add such patch to your series?
I'll proceed with my work on top of your series and will ignore this
race for now, but I think it should be fixed before we land this set
into multiple trees.

Tetsuo Handa June 30, 2020, 9:54 p.m. UTC | #22

On 2020/07/01 1:48, Alexei Starovoitov wrote:
> On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
>> On 2020/06/30 5:19, Eric W. Biederman wrote:
>>> Tetsuo Handa <[e-mail]> writes:
>>>
>>>> On 2020/06/29 4:44, Alexei Starovoitov wrote:
>>>>> But all the defensive programming kinda goes against general kernel style.
>>>>> I wouldn't do it. Especially pr_info() ?!
>>>>> Though I don't feel strongly about it.
>>>>
>>>> Honestly speaking, caller should check for errors and print appropriate
>>>> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
>>>> went wrong (maybe memory corruption). But other conditions are not fatal.
>>>> That is, I consider even pr_info() here should be unnecessary.
>>>
>>> They were all should never happen cases.  Which is why my patches do:
>>> if (WARN_ON_ONCE(...))
>>
>> No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
> 
> I don't believe that's true.
> Please show fuzzing stack trace to prove your point.
> 

Please find links containing "WARNING" from https://syzkaller.appspot.com/upstream . ;-)

Alexei Starovoitov June 30, 2020, 9:57 p.m. UTC | #23

On Tue, Jun 30, 2020 at 2:55 PM Tetsuo Handa
<[e-mail]> wrote:
>
> On 2020/07/01 1:48, Alexei Starovoitov wrote:
> > On Tue, Jun 30, 2020 at 03:28:49PM +0900, Tetsuo Handa wrote:
> >> On 2020/06/30 5:19, Eric W. Biederman wrote:
> >>> Tetsuo Handa <[e-mail]> writes:
> >>>
> >>>> On 2020/06/29 4:44, Alexei Starovoitov wrote:
> >>>>> But all the defensive programming kinda goes against general kernel style.
> >>>>> I wouldn't do it. Especially pr_info() ?!
> >>>>> Though I don't feel strongly about it.
> >>>>
> >>>> Honestly speaking, caller should check for errors and print appropriate
> >>>> messages. info->wd.mnt->mnt_root != info->wd.dentry indicates that something
> >>>> went wrong (maybe memory corruption). But other conditions are not fatal.
> >>>> That is, I consider even pr_info() here should be unnecessary.
> >>>
> >>> They were all should never happen cases.  Which is why my patches do:
> >>> if (WARN_ON_ONCE(...))
> >>
> >> No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
> >
> > I don't believe that's true.
> > Please show fuzzing stack trace to prove your point.
> >
>
> Please find links containing "WARNING" from https://syzkaller.appspot.com/upstream . ;-)

Is it a joke? Do you understand how syzbot works?
If so, please explain how it can invoke umd_* interface.

Tetsuo Handa June 30, 2020, 10:58 p.m. UTC | #24

On 2020/07/01 6:57, Alexei Starovoitov wrote:
>>>>> They were all should never happen cases.  Which is why my patches do:
>>>>> if (WARN_ON_ONCE(...))
>>>>
>>>> No. Fuzz testing (which uses panic_on_warn=1) will trivially hit them.
>>>
>>> I don't believe that's true.
>>> Please show fuzzing stack trace to prove your point.
>>>
>>
>> Please find links containing "WARNING" from https://syzkaller.appspot.com/upstream . ;-)
> 
> Is it a joke? Do you understand how syzbot works?
> If so, please explain how it can invoke umd_* interface.
> 

Currently syzkaller can't invoke umd_* interface because this interface is used by only
bpfilter_umh module. But I can imagine that someone starts using this interface in a way
syzkaller can somehow invoke. Thus, how can it be a joke? I don't understand your question.

Eric W. Biederman July 1, 2020, 5:12 p.m. UTC | #25

Alexei Starovoitov <[e-mail]> writes:

> On Tue, Jun 30, 2020 at 07:29:34AM -0500, Eric W. Biederman wrote:
>> 
>> diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
>> index 91474884ddb7..3e1874030daa 100644
>> --- a/net/bpfilter/bpfilter_kern.c
>> +++ b/net/bpfilter/bpfilter_kern.c
>> @@ -19,8 +19,8 @@ static void shutdown_umh(void)
>>         struct pid *tgid = info->tgid;
>>  
>>         if (tgid) {
>> -               kill_pid_info(SIGKILL, SEND_SIG_PRIV, tgid);
>> -               wait_event(tgid->wait_pidfd, !pid_task(tgid, PIDTYPE_TGID));
>> +               kill_pid(tgid, SIGKILL, 1);
>> +               wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>>                 bpfilter_umh_cleanup(info);
>>         }
>>  }
>> 
>> > And then did:
>> > while true; do iptables -L;rmmod bpfilter; done
>> >  
>> > Unfortunately sometimes 'rmmod bpfilter' hangs in wait_event().
>> 
>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>> release_task is called so there is a race.  As it is possible to wake
>> up and then go back to sleep before pid_has_task becomes false.
>> 
>> So I think I need a friendly helper that does:
>> 
>> bool task_has_exited(struct pid *tgid)
>> {
>> 	bool exited = false;
>> 
>> 	rcu_read_lock();
>>         tsk = pid_task(tgid, PIDTYPE_TGID);
>>         exited = !!tsk;
>>         if (tsk) {
>>         	exited = !!tsk->exit_state;
>> out:
>> 	rcu_unlock();
>> 	return exited;
>> }
>
> All makes sense to me.
> If I understood the race condition such helper should indeed solve it.
> Are you going to add such patch to your series?
> I'll proceed with my work on top of your series and will ignore this
> race for now, but I think it should be fixed before we land this set
> into multiple trees.

Yes. I am just finishing it up now.

Eric

Eric W. Biederman July 2, 2020, 1:08 p.m. UTC | #26

Tetsuo Handa <[e-mail]> writes:

> On 2020/06/30 21:29, Eric W. Biederman wrote:
>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>> release_task is called so there is a race.  As it is possible to wake
>> up and then go back to sleep before pid_has_task becomes false.
>
> What is the reason we want to wait until pid_has_task() becomes false?
>
> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));

So that it is safe to call bpfilter_umh_cleanup.  The previous code
performed the wait by having a callback in do_exit.

It might be possible to call bpf_umh_cleanup early but I have not done
that analysis.

To perform the test correctly what I have right now is:

bool thread_group_exited(struct pid *pid)
{
	struct task_struct *tsk;
	bool exited;

	rcu_read_lock();
	tsk = pid_task(pid, PIDTYPE_PID);
	exited = !tsk || (READ_ONCE(tsk->exit_state) && thread_group_empty(tsk));
	rcu_read_unlock();

	return exited;
}

Which is factored out of pidfd_poll.  Which means that this won't be
something that the bpfilter code has to maintain.  That seems to be a
fundamentally good facility to have regardless of bpfilter.

I will post the whole thing in a bit once I have a chance to dot my i's
and cross my t's.

> By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
> that use of flush_delayed_fput() has to be careful. Al, is it safe to call
> flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
> called from both kernel thread and from process context (e.g. init_module()
> syscall by /sbin/insmod )) ?

And __fput_sync needs to be even more careful.
umd_load_blob is called in these changes without any locks held.

We fundamentally AKA in any correct version of this code need to flush
the file descriptor before we call exec or exec can not open it a
read-only denying all writes from any other opens.

The use case of flush_delayed_fput is exactly the same as that used
when loading the initramfs.

Eric

Tetsuo Handa July 2, 2020, 1:40 p.m. UTC | #27

On 2020/07/02 22:08, Eric W. Biederman wrote:
> Tetsuo Handa <[e-mail]> writes:
> 
>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>>> release_task is called so there is a race.  As it is possible to wake
>>> up and then go back to sleep before pid_has_task becomes false.
>>
>> What is the reason we want to wait until pid_has_task() becomes false?
>>
>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
> 
> So that it is safe to call bpfilter_umh_cleanup.  The previous code
> performed the wait by having a callback in do_exit.

But bpfilter_umh_cleanup() does only

	fput(info->pipe_to_umh);
	fput(info->pipe_from_umh);
	put_pid(info->tgid);
	info->tgid = NULL;

which is (I think) already safe regardless of the usermode process because
bpfilter_umh_cleanup() merely closes one side of two pipes used between
two processes and forgets about the usermode process.

> 
> It might be possible to call bpf_umh_cleanup early but I have not done
> that analysis.
> 
> To perform the test correctly what I have right now is:

Waiting for the termination of a SIGKILLed usermode process is not
such simple. If a usermode process was killed by the OOM killer, it
might take minutes for the killed process to reach do_exit() due to
invisible memory allocation dependency chain. Since the OOM killer
kicks the OOM reaper, and the OOM reaper forgets about the killed
process after one second if mmap_sem could not be held (in order to
avoid OOM deadlock), the OOM situation will be eventually solved; but
there is no guarantee that the killed process can reach do_exit()
in a short period.

Eric W. Biederman July 2, 2020, 4:02 p.m. UTC | #28

Tetsuo Handa <[e-mail]> writes:

> On 2020/07/02 22:08, Eric W. Biederman wrote:
>> Tetsuo Handa <[e-mail]> writes:
>> 
>>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>>>> release_task is called so there is a race.  As it is possible to wake
>>>> up and then go back to sleep before pid_has_task becomes false.
>>>
>>> What is the reason we want to wait until pid_has_task() becomes false?
>>>
>>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
>> 
>> So that it is safe to call bpfilter_umh_cleanup.  The previous code
>> performed the wait by having a callback in do_exit.
>
> But bpfilter_umh_cleanup() does only
>
> 	fput(info->pipe_to_umh);
> 	fput(info->pipe_from_umh);
> 	put_pid(info->tgid);
> 	info->tgid = NULL;
>
> which is (I think) already safe regardless of the usermode process because
> bpfilter_umh_cleanup() merely closes one side of two pipes used between
> two processes and forgets about the usermode process.

It is not safe.

Baring bugs there is only one use of shtudown_umh that matters.  The one
in fini_umh.  The use of the file by the mm must be finished before
umd_unload_blob.  AKA unmount.  Which completely frees the filesystem.

>> It might be possible to call bpf_umh_cleanup early but I have not done
>> that analysis.
>> 
>> To perform the test correctly what I have right now is:
>
> Waiting for the termination of a SIGKILLed usermode process is not
> such simple.

The waiting is that simple.

You are correct it might not be a quick process.

A good general principle is to start with something simple and correct
for what it does, and then to make it more complicated when real world
cases show up, and it can be understood what the real challenges are.

I am not going to merge known broken code but I am also not going to
overcomplicate it.

Dealing with very rare and pathological cases that are not handled or
considered today is out of scope for my patchset.

Eric

Tetsuo Handa July 3, 2020, 1:19 p.m. UTC | #29

On 2020/07/02 22:08, Eric W. Biederman wrote:
>> By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
>> that use of flush_delayed_fput() has to be careful. Al, is it safe to call
>> flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
>> called from both kernel thread and from process context (e.g. init_module()
>> syscall by /sbin/insmod )) ?
> 
> And __fput_sync needs to be even more careful.
> umd_load_blob is called in these changes without any locks held.

But where is the guarantee that a thread which called flush_delayed_fput() waits for
the completion of processing _all_ "struct file" linked into delayed_fput_list ?
If some other thread or delayed_fput_work (scheduled by fput_many()) called
flush_delayed_fput() between blob_to_mnt()'s fput(file) and flush_delayed_fput()
sequence? blob_to_mnt()'s flush_delayed_fput() can miss the "struct file" which
needs to be processed before execve(), can't it?

Also, I don't know how convoluted the dependency of all "struct file" linked into
delayed_fput_list might be, for there can be "struct file" which will not be a
simple close of tmpfs file created by blob_to_mnt()'s file_open_root() request.

On the other hand, although __fput_sync() cannot be called from !PF_KTHREAD threads,
there is a guarantee that __fput_sync() waits for the completion of "struct file"
which needs to be flushed before execve(), isn't there?

> 
> We fundamentally AKA in any correct version of this code need to flush
> the file descriptor before we call exec or exec can not open it a
> read-only denying all writes from any other opens.
> 
> The use case of flush_delayed_fput is exactly the same as that used
> when loading the initramfs.

When loading the initramfs, the number of threads is quite few (which
means that the possibility of hitting the race window and convoluted
dependency is small).

But like EXPORT_SYMBOL_GPL(umd_load_blob) indicates, blob_to_mnt()'s
flush_delayed_fput() might be called after many number of threads already
started running.



On 2020/07/03 1:02, Eric W. Biederman wrote:
>>>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>>>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>>>>> release_task is called so there is a race.  As it is possible to wake
>>>>> up and then go back to sleep before pid_has_task becomes false.
>>>>
>>>> What is the reason we want to wait until pid_has_task() becomes false?
>>>>
>>>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>>>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
>>>
>>> So that it is safe to call bpfilter_umh_cleanup.  The previous code
>>> performed the wait by having a callback in do_exit.
>>
>> But bpfilter_umh_cleanup() does only
>>
>> 	fput(info->pipe_to_umh);
>> 	fput(info->pipe_from_umh);
>> 	put_pid(info->tgid);
>> 	info->tgid = NULL;
>>
>> which is (I think) already safe regardless of the usermode process because
>> bpfilter_umh_cleanup() merely closes one side of two pipes used between
>> two processes and forgets about the usermode process.
> 
> It is not safe.
> 
> Baring bugs there is only one use of shtudown_umh that matters.  The one
> in fini_umh.  The use of the file by the mm must be finished before
> umd_unload_blob.  AKA unmount.  Which completely frees the filesystem.

Do we really need to mount upon umd_load_blob() and unmount upon umd_unload_blob() ?
LSM modules might prefer only one instance of filesystem for umd blobs.

For pathname based LSMs, since that filesystem is not visible from mount tree, only
info->driver_name can be used for distinction. Therefore, one instance of filesystem
with files created with file_open_root(O_CREAT | O_WRONLY | O_EXCL) might be preferable.

For inode based LSMs, reusing one instance of filesystem created upon early boot might
be convenient for labeling.

Also, we might want a dedicated filesystem (say, "umdfs") instead of regular tmpfs in
order to implement protections without labeling files. Then, we might also be able to
implement minimal protections without LSMs.

Eric W. Biederman July 3, 2020, 10:25 p.m. UTC | #30

Tetsuo Handa <[e-mail]> writes:

> On 2020/07/02 22:08, Eric W. Biederman wrote:
>>> By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
>>> that use of flush_delayed_fput() has to be careful. Al, is it safe to call
>>> flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
>>> called from both kernel thread and from process context (e.g. init_module()
>>> syscall by /sbin/insmod )) ?
>> 
>> And __fput_sync needs to be even more careful.
>> umd_load_blob is called in these changes without any locks held.
>
> But where is the guarantee that a thread which called flush_delayed_fput() waits for
> the completion of processing _all_ "struct file" linked into delayed_fput_list ?
> If some other thread or delayed_fput_work (scheduled by fput_many()) called
> flush_delayed_fput() between blob_to_mnt()'s fput(file) and flush_delayed_fput()
> sequence? blob_to_mnt()'s flush_delayed_fput() can miss the "struct file" which
> needs to be processed before execve(), can't it?

As a module the guarantee is we call task_work_run.
Built into the kernel the guarantee as best I can trace it is that
kthreadd hasn't started, and as such nothing that is scheduled has run
yet.

> Also, I don't know how convoluted the dependency of all "struct file" linked into
> delayed_fput_list might be, for there can be "struct file" which will not be a
> simple close of tmpfs file created by blob_to_mnt()'s file_open_root() request.
>
> On the other hand, although __fput_sync() cannot be called from !PF_KTHREAD threads,
> there is a guarantee that __fput_sync() waits for the completion of "struct file"
> which needs to be flushed before execve(), isn't there?

There is really not a good helper or helpers, and this code suggests we
have something better.  Right now I have used the existing helpers to
the best of my ability.  If you or someone else wants to write a better
version of flushing so that exec can happen be my guest.

As far as I can tell what I have is good enough.

>> We fundamentally AKA in any correct version of this code need to flush
>> the file descriptor before we call exec or exec can not open it a
>> read-only denying all writes from any other opens.
>> 
>> The use case of flush_delayed_fput is exactly the same as that used
>> when loading the initramfs.
>
> When loading the initramfs, the number of threads is quite few (which
> means that the possibility of hitting the race window and convoluted
> dependency is small).

But the reality is the code run very early, before the initramfs is
initialized in practice.

> But like EXPORT_SYMBOL_GPL(umd_load_blob) indicates, blob_to_mnt()'s
> flush_delayed_fput() might be called after many number of threads already
> started running.

At which point the code probably won't be runnig from a kernel thread
but instead will be running in a thread where task_work_run is relevant.

At worst it is a very small race, where someone else in another thread
starts flushing the file.  Which means the file could still be
completely close before exec.   Even that is not necessarily fatal,
as the usermode driver code has a respawn capability.

Code that is used enough that it hits that race sounds like a very
good problem to have from the perspective of the usermode driver code.

> Do we really need to mount upon umd_load_blob() and unmount upon umd_unload_blob() ?
> LSM modules might prefer only one instance of filesystem for umd
> blobs.

It is simple. People are free to change it, but a single filesystem
seems like a very good place to start with this functionality.

> For pathname based LSMs, since that filesystem is not visible from mount tree, only
> info->driver_name can be used for distinction. Therefore, one instance of filesystem
> with files created with file_open_root(O_CREAT | O_WRONLY | O_EXCL)
> might be preferable.

I took a quick look and the creation and removal of files with the
in-kernel helpers is not particularly easy.  Certainly it is more work
and thus a higher likelyhood of bugs than what I have done.

A directory per driver does sound tempting.  Just more work that I am
willing to do.

> For inode based LSMs, reusing one instance of filesystem created upon early boot might
> be convenient for labeling.
>
> Also, we might want a dedicated filesystem (say, "umdfs") instead of regular tmpfs in
> order to implement protections without labeling files. Then, we might also be able to
> implement minimal protections without LSMs.

All valid points.  Nothing sets this design in stone.
Nothing says this is the endpoint of the evolution of this code.

The entire point of this patchset for me is that I remove the
unnecessary special cases from exec and do_exit, so I don't have to deal
with the usermode driver code anymore.

Eric

Tetsuo Handa July 4, 2020, 6:57 a.m. UTC | #31

On 2020/07/04 7:25, Eric W. Biederman wrote:
> Tetsuo Handa <[e-mail]> writes:
> 
>> On 2020/07/02 22:08, Eric W. Biederman wrote:
>>>> By the way, commit 4a9d4b024a3102fc ("switch fput to task_work_add") says
>>>> that use of flush_delayed_fput() has to be careful. Al, is it safe to call
>>>> flush_delayed_fput() from blob_to_mnt() from umd_load_blob() (which might be
>>>> called from both kernel thread and from process context (e.g. init_module()
>>>> syscall by /sbin/insmod )) ?
>>>
>>> And __fput_sync needs to be even more careful.
>>> umd_load_blob is called in these changes without any locks held.
>>
>> But where is the guarantee that a thread which called flush_delayed_fput() waits for
>> the completion of processing _all_ "struct file" linked into delayed_fput_list ?
>> If some other thread or delayed_fput_work (scheduled by fput_many()) called
>> flush_delayed_fput() between blob_to_mnt()'s fput(file) and flush_delayed_fput()
>> sequence? blob_to_mnt()'s flush_delayed_fput() can miss the "struct file" which
>> needs to be processed before execve(), can't it?
> 
> As a module the guarantee is we call task_work_run.

No. It is possible that blob_to_mnt() is called by a kernel thread which was
started by init_module() syscall by /sbin/insmod .

> Built into the kernel the guarantee as best I can trace it is that
> kthreadd hasn't started, and as such nothing that is scheduled has run
> yet.

Have you ever checked how early the kthreadd (PID=2) gets started?

----------
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2306,6 +2306,7 @@ static __latent_entropy struct task_struct *copy_process(
        trace_task_newtask(p, clone_flags);
        uprobe_copy_process(p, clone_flags);

+       printk(KERN_INFO "Created PID: %u Comm: %s\n", p->pid, p->comm);
        return p;

 bad_fork_cancel_cgroup:
----------

----------
[    0.090757][    T0] pid_max: default: 65536 minimum: 512
[    0.090890][    T0] LSM: Security Framework initializing
[    0.090890][    T0] Mount-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.090890][    T0] Mountpoint-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.090890][    T0] Disabled fast string operations
[    0.090890][    T0] Last level iTLB entries: 4KB 1024, 2MB 1024, 4MB 1024
[    0.090890][    T0] Last level dTLB entries: 4KB 1024, 2MB 1024, 4MB 1024, 1GB 4
[    0.090890][    T0] Spectre V1 : Mitigation: usercopy/swapgs barriers and __user pointer sanitization
[    0.090890][    T0] Spectre V2 : Spectre mitigation: kernel not compiled with retpoline; no mitigation available!
[    0.090890][    T0] Speculative Store Bypass: Mitigation: Speculative Store Bypass disabled via prctl and seccomp
[    0.090890][    T0] SRBDS: Unknown: Dependent on hypervisor status
[    0.090890][    T0] MDS: Mitigation: Clear CPU buffers
[    0.090890][    T0] Freeing SMP alternatives memory: 24K
[    0.090890][    T0] Created PID: 1 Comm: swapper/0
[    0.090890][    T0] Created PID: 2 Comm: swapper/0
[    0.090890][    T1] smpboot: CPU0: Intel(R) Core(TM) i5-4440S CPU @ 2.80GHz (family: 0x6, model: 0x3c, stepping: 0x3)
[    0.091000][    T2] Created PID: 3 Comm: kthreadd
[    0.091995][    T2] Created PID: 4 Comm: kthreadd
[    0.093028][    T2] Created PID: 5 Comm: kthreadd
[    0.093997][    T2] Created PID: 6 Comm: kthreadd
[    0.094995][    T2] Created PID: 7 Comm: kthreadd
[    0.096037][    T2] Created PID: 8 Comm: kthreadd
(...snipped...)
[    0.135716][    T2] Created PID: 13 Comm: kthreadd
[    0.135716][    T1] smp: Bringing up secondary CPUs ...
[    0.135716][    T2] Created PID: 14 Comm: kthreadd
[    0.135716][    T2] Created PID: 15 Comm: kthreadd
[    0.135716][    T2] Created PID: 16 Comm: kthreadd
[    0.135716][    T2] Created PID: 17 Comm: kthreadd
[    0.135716][    T2] Created PID: 18 Comm: kthreadd
[    0.135716][    T1] x86: Booting SMP configuration:
(...snipped...)
[    0.901990][    T1] pci 0000:00:00.0: Limiting direct PCI/PCI transfers
[    0.902145][    T1] pci 0000:00:0f.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff]
[    0.902213][    T1] pci 0000:02:00.0: CLS mismatch (32 != 64), using 64 bytes
[    0.902224][    T1] Trying to unpack rootfs image as initramfs...
[    1.107993][    T1] Freeing initrd memory: 18876K
[    1.109049][    T1] PCI-DMA: Using software bounce buffering for IO (SWIOTLB)
[    1.111003][    T1] software IO TLB: mapped [mem 0xab000000-0xaf000000] (64MB)
[    1.112136][    T1] check: Scanning for low memory corruption every 60 seconds
[    1.115040][    T2] Created PID: 52 Comm: kthreadd
[    1.116110][    T1] workingset: timestamp_bits=46 max_order=20 bucket_order=0
[    1.120936][    T1] SGI XFS with ACLs, security attributes, verbose warnings, quota, no debug enabled
[    1.129626][    T2] Created PID: 53 Comm: kthreadd
[    1.131403][    T2] Created PID: 54 Comm: kthreadd
----------

kthreadd (PID=2) is created by swapper/0 (PID=0) immediately after init (PID=1) was created by
swapper/0 (PID=0). It is even before secondary CPUs are brought up, and far earlier than unpacking
initramfs.

And how can we prove that blob_to_mnt() is only called by a kernel thread before some kernel
thread that interferes fput() starts running? blob_to_mnt() needs to be prepared for being
called after many processes already started running.

> 
>> Also, I don't know how convoluted the dependency of all "struct file" linked into
>> delayed_fput_list might be, for there can be "struct file" which will not be a
>> simple close of tmpfs file created by blob_to_mnt()'s file_open_root() request.
>>
>> On the other hand, although __fput_sync() cannot be called from !PF_KTHREAD threads,
>> there is a guarantee that __fput_sync() waits for the completion of "struct file"
>> which needs to be flushed before execve(), isn't there?
> 
> There is really not a good helper or helpers, and this code suggests we
> have something better.  Right now I have used the existing helpers to
> the best of my ability.  If you or someone else wants to write a better
> version of flushing so that exec can happen be my guest.
> 
> As far as I can tell what I have is good enough.

Just saying what you think is not a "review". I'm waiting for answer from Al Viro
because I consider that Al will be the most familiar with fput()'s behavior.
At least I consider that

	if (current->flags & PF_KTHREAD) {
		__fput_sync(file);
	} else {
		fput(file);
		task_work_run();
	}

is a candidate for closing the race window. And depending on Al's answer,
removing

	BUG_ON(!(task->flags & PF_KTHREAD));

 from __fput_sync() and unconditionally using

	__fput_sync(file);

 from blob_to_mnt() might be the better choice. Anyway, I consider that
Al's response is important for this "review".

> 
>>> We fundamentally AKA in any correct version of this code need to flush
>>> the file descriptor before we call exec or exec can not open it a
>>> read-only denying all writes from any other opens.
>>>
>>> The use case of flush_delayed_fput is exactly the same as that used
>>> when loading the initramfs.
>>
>> When loading the initramfs, the number of threads is quite few (which
>> means that the possibility of hitting the race window and convoluted
>> dependency is small).
> 
> But the reality is the code run very early, before the initramfs is
> initialized in practice.

Such expectation is not a reality.

> 
>> But like EXPORT_SYMBOL_GPL(umd_load_blob) indicates, blob_to_mnt()'s
>> flush_delayed_fput() might be called after many number of threads already
>> started running.
> 
> At which point the code probably won't be runnig from a kernel thread
> but instead will be running in a thread where task_work_run is relevant.

No. It is possible that blob_to_mnt() is called by a kernel thread which was
started by init_module() syscall by /sbin/insmod .

> 
> At worst it is a very small race, where someone else in another thread
> starts flushing the file.  Which means the file could still be
> completely close before exec.   Even that is not necessarily fatal,
> as the usermode driver code has a respawn capability.
> 
> Code that is used enough that it hits that race sounds like a very
> good problem to have from the perspective of the usermode driver code.

In general, unconditionally retrying call_usermodehelper() when it returned
a negative value (e.g. -ENOENT, -ENOMEM, -EBUSY) is bad. I don't know which
code is an implementation of "a respawn capability"; I'd like to check where
that code is and whether that code is checking -ETXTBSY.

Eric W. Biederman July 8, 2020, 4:46 a.m. UTC | #32

Just to make certain I understand what is going on I instrumented a
kernel with some print statements.

a) The workqueues and timers start before populate_rootfs.

b) populate_rootfs does indeed happen long before the bpfilter
   module is intialized.

c) What prevents populate_rootfs and the umd_load_blob from
   having problems when they call flush_delayed_put is the
   fact that fput_many does:
   "schedule_delayed_work(&delayed_fput_work,1)".

   That 1 requests a delay of at least 1 jiffy.  A jiffy is between
   1ms and 10ms depending on how Linux is configured.

   In my test configuration running a kernel in kvm printing to a serial
   console I measured 0.8ms between the fput in blob_to_mnt and
   flush_delayed_fput which immediately follows it.

   So unless the fput becomes incredibly slow there is nothing to worry
   about in blob_to_mnt.

d) As the same mechanism is used by populate_rootfs.  A but in the
   mechanism applies to both.

e) No one appears to have reported a problem executing files out of
   initramfs these last several years since the flush_delayed_fput was
   introduced.
 
f) The code works for me.  There is real reason to believe the code will
   work for everyone else, as the exact same logic is used by initramfs.
   So it should be perfectly fine for the patchset and the
   usermode_driver code to go ahead as written.

h) If there is something to be fixed it is flush_delayed_fput as that is
   much more important than anything in the usermode driver code.

Eric

p.s.) When I talked of restarts of the usermode driver code ealier I was
   referring to the code that restarts the usermode driver if it is
   killed, the next time the kernel tries to talk to it.

   That could mask an -ETXTBUSY except if it happens on the first exec
   the net/bfilter/bpfilter_kern.c:load_umh() will return an error.

Luis Chamberlain July 8, 2020, 5:20 a.m. UTC | #33

On Mon, Jun 29, 2020 at 02:55:05PM -0500, Eric W. Biederman wrote:
> 
> 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.
> 
> I have compiled tested each change with and without CONFIG_BPFILTER
> enabled.

Sounds like grounds for a selftests driver and respective selftest?
And if so, has the other issues Tetsuo reported be hacked into one?

  Luis