net: bpfilter: restart bpfilter_umh when error occurred

PATCH In review Taehee Yoo 2 participants 7 messages Dec 12, 2018 – Dec 19, 2018

Subsystems (4)

Networking Network drivers BPF BPF core

lore

Share & tools

lore

Taehee Yoo

author

2018-12-12 01:19 UTC #0 lore raw

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

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 <[email protected]>

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

include/linux/bpfilter.h +2 -0

index f02cee0225d4..3039361cd65a 100644
@@ -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

net/bpfilter/bpfilter_kern.c +11 -1

index 7acfc83087d5..7397f5e8da2f 100644
@@ -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);

net/bpfilter/bpfilter_umh_blob.S +1 -1

index 40311d10d2f2..7f1c521dcc2f 100644
@@ -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"

net/ipv4/bpfilter/sockopt.c +5 -1

index 5e04ed25bc0e..f7efcff9634d 100644
@@ -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);
 }

Signature · 1 line

2.17.1

David Miller

maintainer

2018-12-15 00:58 UTC #1 lore raw

From: Taehee Yoo <[email protected]>
Date: Wed, 12 Dec 2018 10:19:26 +0900

Quoted 14 lines

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

Quoted 8 lines

   $ 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 <[email protected]>

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.

Taehee Yoo

author

2018-12-15 04:22 UTC #2 lore raw

Quoted 22 lines · David Miller

From: Taehee Yoo <[email protected]>
Date: Wed, 12 Dec 2018 10:19:26 +0900
Quoted 18 lines
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.

Quoted 7 lines

Quoted 4 lines
Then, iptables command is always failed.
Fixes d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
Signed-off-by Taehee Yoo <[email protected]>
Thank you for this fix, but I am unsure if this is a complete solution.

Thank you for your review!

Quoted 2 lines

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

Yes, only root could kill bpfilter_umh process.

Quoted 8 lines

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.

Quoted 7 lines

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.

David Miller

maintainer

2018-12-16 22:20 UTC #3 lore raw

From: Taehee Yoo <[email protected]>
Date: Sat, 15 Dec 2018 13:22:39 +0900

Quoted 5 lines

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:

Quoted 4 lines

   $ 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?

Taehee Yoo

author

2018-12-17 07:31 UTC #4 lore raw

Quoted 32 lines · David Miller

From: Taehee Yoo <[email protected]>
Date: Sat, 15 Dec 2018 13:22:39 +0900
Quoted 5 lines
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:
Quoted 4 lines
Quoted 4 lines
   $ 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.

Quoted 5 lines

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

David Miller

maintainer

2018-12-18 22:51 UTC #5 lore raw

From: Taehee Yoo <[email protected]>
Date: Mon, 17 Dec 2018 16:31:04 +0900

Quoted 2 lines

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?

Taehee Yoo

author

2018-12-19 08:03 UTC #6 lore raw

Quoted 7 lines · David Miller

From: Taehee Yoo <[email protected]>
Date: Mon, 17 Dec 2018 16:31:04 +0900
Quoted 2 lines
But I don't know how cleanup callback is invoked when bpfilter_umh process is killed.

Quoted 9 lines

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!