[PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[Taehee Yoo]

When bpfilter error occurred bpfilter_umh will be stopped via __stop_umh().
The bpfilter_umh() couldn't start again because there is no restart
routine.

The section of bpfilter_umh_{start/end} is no longer .init.rodata
because these area should be reused in the restart routine. hence
the section name is changed to .bpfilter_umh.

Test commands:
   $ iptables -vnL
   $ kill -9 <pid of bpfilter_umh>
   $ iptables -vnL
   [  480.045136] bpfilter: write fail -32

   $ iptables -vnL
   iptables v1.8.1 (legacy): can't initialize iptables table `filter': No child processes
   Perhaps iptables or your kernel needs to be upgraded.

Then, iptables command is always failed.

Fixes: d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 include/linux/bpfilter.h         |  2 ++
 net/bpfilter/bpfilter_kern.c     | 12 +++++++++++-
 net/bpfilter/bpfilter_umh_blob.S |  2 +-
 net/ipv4/bpfilter/sockopt.c      |  6 +++++-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpfilter.h b/include/linux/bpfilter.h
index f02cee0225d4..3039361cd65a 100644
--- a/include/linux/bpfilter.h
+++ b/include/linux/bpfilter.h
@@ -12,4 +12,6 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, char __user *optval,
 extern int (*bpfilter_process_sockopt)(struct sock *sk, int optname,
 				       char __user *optval,
 				       unsigned int optlen, bool is_set);
+extern int (*bpfilter_start_umh)(void);
+
 #endif
diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index 7acfc83087d5..7397f5e8da2f 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -87,7 +87,7 @@ static int __bpfilter_process_sockopt(struct sock *sk, int optname,
 	return ret;
 }
 
-static int __init load_umh(void)
+int start_umh(void)
 {
 	int err;
 
@@ -111,8 +111,18 @@ static int __init load_umh(void)
 	return 0;
 }
 
+static int __init load_umh(void)
+{
+	if (IS_ENABLED(CONFIG_INET))
+		bpfilter_start_umh = &start_umh;
+
+	return start_umh();
+}
+
 static void __exit fini_umh(void)
 {
+	if (IS_ENABLED(CONFIG_INET))
+		bpfilter_start_umh = NULL;
 	stop_umh();
 }
 module_init(load_umh);
diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
index 40311d10d2f2..7f1c521dcc2f 100644
--- a/net/bpfilter/bpfilter_umh_blob.S
+++ b/net/bpfilter/bpfilter_umh_blob.S
@@ -1,5 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-	.section .init.rodata, "a"
+	.section .bpfilter_umh, "a"
 	.global bpfilter_umh_start
 bpfilter_umh_start:
 	.incbin "net/bpfilter/bpfilter_umh"
diff --git a/net/ipv4/bpfilter/sockopt.c b/net/ipv4/bpfilter/sockopt.c
index 5e04ed25bc0e..f7efcff9634d 100644
--- a/net/ipv4/bpfilter/sockopt.c
+++ b/net/ipv4/bpfilter/sockopt.c
@@ -10,6 +10,9 @@ int (*bpfilter_process_sockopt)(struct sock *sk, int optname,
 				unsigned int optlen, bool is_set);
 EXPORT_SYMBOL_GPL(bpfilter_process_sockopt);
 
+int (*bpfilter_start_umh)(void);
+EXPORT_SYMBOL_GPL(bpfilter_start_umh);
+
 static int bpfilter_mbox_request(struct sock *sk, int optname,
 				 char __user *optval,
 				 unsigned int optlen, bool is_set)
@@ -20,7 +23,8 @@ static int bpfilter_mbox_request(struct sock *sk, int optname,
 		if (err)
 			return err;
 		if (!bpfilter_process_sockopt)
-			return -ECHILD;
+			if (!bpfilter_start_umh || bpfilter_start_umh())
+				return -ECHILD;
 	}
 	return bpfilter_process_sockopt(sk, optname, optval, optlen, is_set);
 }
-- 
2.17.1

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[David Miller]

From: Taehee Yoo <ap420073@gmail.com>
Date: Wed, 12 Dec 2018 10:19:26 +0900

> When bpfilter error occurred bpfilter_umh will be stopped via __stop_umh().
> The bpfilter_umh() couldn't start again because there is no restart
> routine.
> 
> The section of bpfilter_umh_{start/end} is no longer .init.rodata
> because these area should be reused in the restart routine. hence
> the section name is changed to .bpfilter_umh.
> 
> Test commands:
>    $ iptables -vnL
>    $ kill -9 <pid of bpfilter_umh>
>    $ iptables -vnL
>    [  480.045136] bpfilter: write fail -32
> 
>    $ iptables -vnL
>    iptables v1.8.1 (legacy): can't initialize iptables table `filter': No child processes
>    Perhaps iptables or your kernel needs to be upgraded.
> 
> Then, iptables command is always failed.
> 
> Fixes: d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>

Thank you for this fix, but I am unsure if this is a complete solution.

First of all, you can only kill the bpfilter_umh as root right?

It is a big problem to allow the userspace to kill the umh program
because that will result in the pipes being leaked.  Normally the
kernel takes down bpfilter_umh and calls fput() on the pipe file
descriptors via shutdown_umh().

In the kill -9 example above, that does not happen and that is why
we get the fd leak.

In this situation it will not reset info->pid to zero, and it also
will leave bpfilter_process_socktop non-NULL.

Therefore, it seems like the kernel has to perform these cleanup
actions if the bpfilter_umh process dies (from kill -9, or just
crashing).  And I think if you manage that properly it will fix this
bug too.

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[Taehee Yoo]

On Sat, 15 Dec 2018 at 09:58, David Miller <davem@davemloft.net> wrote:
>
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Wed, 12 Dec 2018 10:19:26 +0900
>
> > When bpfilter error occurred bpfilter_umh will be stopped via __stop_umh().
> > The bpfilter_umh() couldn't start again because there is no restart
> > routine.
> >
> > The section of bpfilter_umh_{start/end} is no longer .init.rodata
> > because these area should be reused in the restart routine. hence
> > the section name is changed to .bpfilter_umh.
> >
> > Test commands:
> >    $ iptables -vnL
> >    $ kill -9 <pid of bpfilter_umh>
> >    $ iptables -vnL
> >    [  480.045136] bpfilter: write fail -32
> >
> >    $ iptables -vnL
> >    iptables v1.8.1 (legacy): can't initialize iptables table `filter': No child processes
> >    Perhaps iptables or your kernel needs to be upgraded.
> >
> > Then, iptables command is always failed.
> >
> > Fixes: d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>
> Thank you for this fix, but I am unsure if this is a complete solution.
>

Thank you for your review!

> First of all, you can only kill the bpfilter_umh as root right?
>

Yes, only root could kill bpfilter_umh process.

> It is a big problem to allow the userspace to kill the umh program
> because that will result in the pipes being leaked.  Normally the
> kernel takes down bpfilter_umh and calls fput() on the pipe file
> descriptors via shutdown_umh().
>
> In the kill -9 example above, that does not happen and that is why
> we get the fd leak.
>
> In this situation it will not reset info->pid to zero, and it also
> will leave bpfilter_process_socktop non-NULL.
>
> Therefore, it seems like the kernel has to perform these cleanup
> actions if the bpfilter_umh process dies (from kill -9, or just
> crashing).  And I think if you manage that properly it will fix this
> bug too.

If bpfilter_umh process is killed, shutdown_umh() is executed via __stop_umh().
because, __kernel_write() or kernel_read() will be failed in
__bpfilter_process_sockopt() if bpfilter_umh process had killed
or crashed. then, __bpfilter_process_sockopt() makes error message and
calls __stop_umh().
So the cleanup code is called when next iptables command is executed.

   $modprobe bpfilter
   $kill < pid of bpfilter_umh >
   $iptables -vnL   <-- stop_umh() is called and iptables command
fails at this point.
[  512.543626] bpfilter: write fail -32
   $iptables -vnL   <-- re-start routine will be called and iptables
command will success.

By any chance,  should the cleanup action immediately be called
when the bpfilter_umh process is killed?
If you think that I didn't understand correctly what you said,
please let me know.

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[David Miller]

From: Taehee Yoo <ap420073@gmail.com>
Date: Sat, 15 Dec 2018 13:22:39 +0900

> If bpfilter_umh process is killed, shutdown_umh() is executed via __stop_umh().
> because, __kernel_write() or kernel_read() will be failed in
> __bpfilter_process_sockopt() if bpfilter_umh process had killed
> or crashed. then, __bpfilter_process_sockopt() makes error message and
> calls __stop_umh().

Now I understand, thank you.

This is what happens in the second command of your example:

> >    $ iptables -vnL
> >    $ kill -9 <pid of bpfilter_umh>
> >    $ iptables -vnL
> >    [  480.045136] bpfilter: write fail -32

This second iptables command, which fails, triggers the cleanup.

This second iptables command, however, should not fail either.

What should happen is that when bpfilter_umh is killed, the cleanup is
synchronous, and the next iptables command will cleanly restart
bpftiler_umh and the command will succeeed.

Perhaps what should happen is that fork_usermode_blob() somehow
registers a mechanism by which if the the process forked dies
or exits for some reason, an installed callback is invoked to
perform cleanups.

That would solve all of these problems, and all three iptables
commands in your example would succeed.

What do you think?

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[Taehee Yoo]

On Mon, 17 Dec 2018 at 07:20, David Miller <davem@davemloft.net> wrote:
>
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Sat, 15 Dec 2018 13:22:39 +0900
>
> > If bpfilter_umh process is killed, shutdown_umh() is executed via __stop_umh().
> > because, __kernel_write() or kernel_read() will be failed in
> > __bpfilter_process_sockopt() if bpfilter_umh process had killed
> > or crashed. then, __bpfilter_process_sockopt() makes error message and
> > calls __stop_umh().
>
> Now I understand, thank you.
>
> This is what happens in the second command of your example:
>
> > >    $ iptables -vnL
> > >    $ kill -9 <pid of bpfilter_umh>
> > >    $ iptables -vnL
> > >    [  480.045136] bpfilter: write fail -32
>
> This second iptables command, which fails, triggers the cleanup.
>
> This second iptables command, however, should not fail either.
>
> What should happen is that when bpfilter_umh is killed, the cleanup is
> synchronous, and the next iptables command will cleanly restart
> bpftiler_umh and the command will succeeed.
>
> Perhaps what should happen is that fork_usermode_blob() somehow
> registers a mechanism by which if the the process forked dies
> or exits for some reason, an installed callback is invoked to
> perform cleanups.
>
> That would solve all of these problems, and all three iptables
> commands in your example would succeed.
>
> What do you think?
>

I agree with second iptables should not fail.
I think calling cleanup callback in usermodehelper will be userful
for other modules which uses fork_usermodehelper_blob().
So the usermodehelper should support to invoke cleanup callback when
error or crash occurred.
But I don't know how cleanup callback is invoked when
bpfilter_umh process is killed.
Could you let me know if it's possible?
If it is not possible, In order to avoid failure all iptables command,
I think below steps are needed.
1. check process status
2. if process was dead or crashed, cleanup and restart bpfilter_umh
3. perform normal routine

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[David Miller]

From: Taehee Yoo <ap420073@gmail.com>
Date: Mon, 17 Dec 2018 16:31:04 +0900

> But I don't know how cleanup callback is invoked when
> bpfilter_umh process is killed.

I am suggesting that a new piece of generic infrastructure might
be needed, but it would need to be carefully designed.

The task_struct would get a piece of state, and at exit() time
the kernel would check that state and use it to invoke exit()
time cleanups for UMH type processes.

Is the idea clearer now?

Re: [PATCH net 1/2] net: bpfilter: restart bpfilter_umh when error occurred

[Taehee Yoo]

On Wed, 19 Dec 2018 at 07:51, David Miller <davem@davemloft.net> wrote:
>
> From: Taehee Yoo <ap420073@gmail.com>
> Date: Mon, 17 Dec 2018 16:31:04 +0900
>
> > But I don't know how cleanup callback is invoked when
> > bpfilter_umh process is killed.
>
> I am suggesting that a new piece of generic infrastructure might
> be needed, but it would need to be carefully designed.
>
> The task_struct would get a piece of state, and at exit() time
> the kernel would check that state and use it to invoke exit()
> time cleanups for UMH type processes.
>
> Is the idea clearer now?
>

This idea is clear and I will try to make an infrastructure code
to clean up when UMH type process is killed.

Thanks a lot for the suggestion!