Merge branch 'bpfilter-fixes'

Taehee Yoo says:

====================
net: bpfilter: fix two bugs in bpfilter

This patches fix two bugs in the bpfilter_umh which are related in
iptables command.

The first patch adds an exit code for UMH process.
This provides an opportunity to cleanup members of the umh_info
to modules which use the UMH.
In order to identify UMH processes, a new flag PF_UMH is added.

The second patch makes the bpfilter_umh use UMH cleanup callback.

The third patch adds re-start routine for the bpfilter_umh.
The bpfilter_umh does not re-start after error occurred.
because there is no re-start routine in the module.

The fourth patch ensures that the bpfilter.ko module will not removed while
it's being used.
The bpfilter.ko is not protected by locks or module reference counter.
Therefore that can be removed while module is being used.
In order to protect that, mutex is used.

The first and second patch are preparation patches for the third and
fourth patch.

TEST #1
   while :
   do
	modprobe bpfilter
	kill -9 <pid of the bpfilter_umh>
	iptables -vnL
   done

TEST #2
   while :
   do
	iptables -I FORWARD -m string --string ap --algo kmp &
	iptables -F &
	modprobe -rv bpfilter &
   done

TEST #3
   while :
   do
	modprobe bpfilter &
	modprobe -rv bpfilter &
   done

The TEST1 makes a failure of iptables command.
This is fixed by the third patch.

The TEST2 makes a panic because of a race condition in the bpfilter_umh
module.
This is fixed by the fourth patch.

The TEST3 makes a double-create UMH process.
This is fixed by the third and fourth patch.

v4 :
 - declare the exit_umh() as static inline
 - check stop flag in the load_umh() to avoid a double-create UMH
v3 :
 - Avoid unnecessary list lookup for non-UMH processes
 - Add a new PF_UMH flag
v2 : add the first and second patch
v1 : Initial patch
====================

Signed-off-by: David S. Miller <davem@davemloft.net>

8 files changed, 146 insertions, 50 deletions

diff --git a/include/linux/bpfilter.h b/include/linux/bpfilter.h
index f02cee0225d4..d815622cd31e 100644
--- a/include/linux/bpfilter.h
+++ b/include/linux/bpfilter.h
@@ -3,13 +3,22 @@
 #define _LINUX_BPFILTER_H
 
 #include <uapi/linux/bpfilter.h>
+#include <linux/umh.h>
 
 struct sock;
 int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char __user *optval,
                             unsigned int optlen);
 int bpfilter_ip_get_sockopt(struct sock *sk, int optname, char __user *optval,
                             int __user *optlen);
-extern int (*bpfilter_process_sockopt)(struct sock *sk, int optname,
-                                       char __user *optval,
-                                       unsigned int optlen, bool is_set);
+struct bpfilter_umh_ops {
+        struct umh_info info;
+        /* since ip_getsockopt() can run in parallel, serialize access to umh */
+        struct mutex lock;
+        int (*sockopt)(struct sock *sk, int optname,
+                       char __user *optval,
+                       unsigned int optlen, bool is_set);
+        int (*start)(void);
+        bool stop;
+};
+extern struct bpfilter_umh_ops bpfilter_ops;
 #endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 89541d248893..e35e35b9fc48 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1406,6 +1406,7 @@ extern struct pid *cad_pid;
 #define PF_RANDOMIZE            0x00400000      /* Randomize virtual address space */
 #define PF_SWAPWRITE            0x00800000      /* Allowed to write to swap */
 #define PF_MEMSTALL             0x01000000      /* Stalled due to lack of memory */
+#define PF_UMH                  0x02000000      /* I'm an Usermodehelper process */
 #define PF_NO_SETAFFINITY       0x04000000      /* Userland is not allowed to meddle with cpus_allowed */
 #define PF_MCE_EARLY            0x08000000      /* Early kill for mce process policy */
 #define PF_MUTEX_TESTER         0x20000000      /* Thread belongs to the rt mutex tester */
@@ -1904,6 +1905,14 @@ 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);
diff --git a/include/linux/umh.h b/include/linux/umh.h
index 235f51b62c71..0c08de356d0d 100644
--- a/include/linux/umh.h
+++ b/include/linux/umh.h
@@ -47,6 +47,8 @@ struct umh_info {
         const char *cmdline;
         struct file *pipe_to_umh;
         struct file *pipe_from_umh;
+        struct list_head list;
+        void (*cleanup)(struct umh_info *info);
         pid_t pid;
 };
 int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
diff --git a/kernel/exit.c b/kernel/exit.c
index 8a01b671dc1f..dad70419195c 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -866,6 +866,7 @@ void __noreturn do_exit(long code)
         exit_task_namespaces(tsk);
         exit_task_work(tsk);
         exit_thread(tsk);
+        exit_umh(tsk);
 
         /*
          * Flush inherited counters to the parent - before the parent
diff --git a/kernel/umh.c b/kernel/umh.c
index 0baa672e023c..d937cbad903a 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -37,6 +37,8 @@ static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
 static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
 static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
+static LIST_HEAD(umh_list);
+static DEFINE_MUTEX(umh_list_lock);
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
@@ -100,10 +102,12 @@ static int call_usermodehelper_exec_async(void *data)
         commit_creds(new);
 
         sub_info->pid = task_pid_nr(current);
-        if (sub_info->file)
+        if (sub_info->file) {
                 retval = do_execve_file(sub_info->file,
                                         sub_info->argv, sub_info->envp);
-        else
+                if (!retval)
+                        current->flags |= PF_UMH;
+        } else
                 retval = do_execve(getname_kernel(sub_info->path),
                                    (const char __user *const __user *)sub_info->argv,
                                    (const char __user *const __user *)sub_info->envp);
@@ -517,6 +521,11 @@ int fork_usermode_blob(void *data, size_t len, struct umh_info *info)
                 goto out;
 
         err = call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
+        if (!err) {
+                mutex_lock(&umh_list_lock);
+                list_add(&info->list, &umh_list);
+                mutex_unlock(&umh_list_lock);
+        }
 out:
         fput(file);
         return err;
@@ -679,6 +688,26 @@ static int proc_cap_handler(struct ctl_table *table, int write,
         return 0;
 }
 
+void __exit_umh(struct task_struct *tsk)
+{
+        struct umh_info *info;
+        pid_t pid = tsk->pid;
+
+        mutex_lock(&umh_list_lock);
+        list_for_each_entry(info, &umh_list, list) {
+                if (info->pid == pid) {
+                        list_del(&info->list);
+                        mutex_unlock(&umh_list_lock);
+                        goto out;
+                }
+        }
+        mutex_unlock(&umh_list_lock);
+        return;
+out:
+        if (info->cleanup)
+                info->cleanup(info);
+}
+
 struct ctl_table usermodehelper_table[] = {
         {
                 .procname       = "bset",
diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index 7acfc83087d5..7ee4fea93637 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -13,39 +13,24 @@
 extern char bpfilter_umh_start;
 extern char bpfilter_umh_end;
 
-static struct umh_info info;
-/* since ip_getsockopt() can run in parallel, serialize access to umh */
-static DEFINE_MUTEX(bpfilter_lock);
-
-static void shutdown_umh(struct umh_info *info)
+static void shutdown_umh(void)
 {
         struct task_struct *tsk;
 
-        if (!info->pid)
+        if (bpfilter_ops.stop)
                 return;
-        tsk = get_pid_task(find_vpid(info->pid), PIDTYPE_PID);
+
+        tsk = get_pid_task(find_vpid(bpfilter_ops.info.pid), PIDTYPE_PID);
         if (tsk) {
                 force_sig(SIGKILL, tsk);
                 put_task_struct(tsk);
         }
-        fput(info->pipe_to_umh);
-        fput(info->pipe_from_umh);
-        info->pid = 0;
 }
 
 static void __stop_umh(void)
 {
-        if (IS_ENABLED(CONFIG_INET)) {
-                bpfilter_process_sockopt = NULL;
-                shutdown_umh(&info);
-        }
-}
-
-static void stop_umh(void)
-{
-        mutex_lock(&bpfilter_lock);
-        __stop_umh();
-        mutex_unlock(&bpfilter_lock);
+        if (IS_ENABLED(CONFIG_INET))
+                shutdown_umh();
 }
 
 static int __bpfilter_process_sockopt(struct sock *sk, int optname,
@@ -63,10 +48,10 @@ static int __bpfilter_process_sockopt(struct sock *sk, int optname,
         req.cmd = optname;
         req.addr = (long __force __user)optval;
         req.len = optlen;
-        mutex_lock(&bpfilter_lock);
-        if (!info.pid)
+        if (!bpfilter_ops.info.pid)
                 goto out;
-        n = __kernel_write(info.pipe_to_umh, &req, sizeof(req), &pos);
+        n = __kernel_write(bpfilter_ops.info.pipe_to_umh, &req, sizeof(req),
+                           &pos);
         if (n != sizeof(req)) {
                 pr_err("write fail %zd\n", n);
                 __stop_umh();
@@ -74,7 +59,8 @@ static int __bpfilter_process_sockopt(struct sock *sk, int optname,
                 goto out;
         }
         pos = 0;
-        n = kernel_read(info.pipe_from_umh, &reply, sizeof(reply), &pos);
+        n = kernel_read(bpfilter_ops.info.pipe_from_umh, &reply, sizeof(reply),
+                        &pos);
         if (n != sizeof(reply)) {
                 pr_err("read fail %zd\n", n);
                 __stop_umh();
@@ -83,37 +69,59 @@ static int __bpfilter_process_sockopt(struct sock *sk, int optname,
         }
         ret = reply.status;
 out:
-        mutex_unlock(&bpfilter_lock);
         return ret;
 }
 
-static int __init load_umh(void)
+static int start_umh(void)
 {
         int err;
 
         /* fork usermode process */
-        info.cmdline = "bpfilter_umh";
         err = fork_usermode_blob(&bpfilter_umh_start,
                                  &bpfilter_umh_end - &bpfilter_umh_start,
-                                 &info);
+                                 &bpfilter_ops.info);
         if (err)
                 return err;
-        pr_info("Loaded bpfilter_umh pid %d\n", info.pid);
+        bpfilter_ops.stop = false;
+        pr_info("Loaded bpfilter_umh pid %d\n", bpfilter_ops.info.pid);
 
         /* health check that usermode process started correctly */
         if (__bpfilter_process_sockopt(NULL, 0, NULL, 0, 0) != 0) {
-                stop_umh();
+                shutdown_umh();
                 return -EFAULT;
         }
-        if (IS_ENABLED(CONFIG_INET))
-                bpfilter_process_sockopt = &__bpfilter_process_sockopt;
 
         return 0;
 }
 
+static int __init load_umh(void)
+{
+        int err;
+
+        mutex_lock(&bpfilter_ops.lock);
+        if (!bpfilter_ops.stop) {
+                err = -EFAULT;
+                goto out;
+        }
+        err = start_umh();
+        if (!err && IS_ENABLED(CONFIG_INET)) {
+                bpfilter_ops.sockopt = &__bpfilter_process_sockopt;
+                bpfilter_ops.start = &start_umh;
+        }
+out:
+        mutex_unlock(&bpfilter_ops.lock);
+        return err;
+}
+
 static void __exit fini_umh(void)
 {
-        stop_umh();
+        mutex_lock(&bpfilter_ops.lock);
+        if (IS_ENABLED(CONFIG_INET)) {
+                shutdown_umh();
+                bpfilter_ops.start = NULL;
+                bpfilter_ops.sockopt = NULL;
+        }
+        mutex_unlock(&bpfilter_ops.lock);
 }
 module_init(load_umh);
 module_exit(fini_umh);
diff --git a/net/bpfilter/bpfilter_umh_blob.S b/net/bpfilter/bpfilter_umh_blob.S
index 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..1e976bb93d99 100644
--- a/net/ipv4/bpfilter/sockopt.c
+++ b/net/ipv4/bpfilter/sockopt.c
@@ -1,28 +1,54 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/uaccess.h>
 #include <linux/bpfilter.h>
 #include <uapi/linux/bpf.h>
 #include <linux/wait.h>
 #include <linux/kmod.h>
+#include <linux/fs.h>
+#include <linux/file.h>
 
-int (*bpfilter_process_sockopt)(struct sock *sk, int optname,
-                                char __user *optval,
-                                unsigned int optlen, bool is_set);
-EXPORT_SYMBOL_GPL(bpfilter_process_sockopt);
+struct bpfilter_umh_ops bpfilter_ops;
+EXPORT_SYMBOL_GPL(bpfilter_ops);
+
+static void bpfilter_umh_cleanup(struct umh_info *info)
+{
+        mutex_lock(&bpfilter_ops.lock);
+        bpfilter_ops.stop = true;
+        fput(info->pipe_to_umh);
+        fput(info->pipe_from_umh);
+        info->pid = 0;
+        mutex_unlock(&bpfilter_ops.lock);
+}
 
 static int bpfilter_mbox_request(struct sock *sk, int optname,
                                  char __user *optval,
                                  unsigned int optlen, bool is_set)
 {
-        if (!bpfilter_process_sockopt) {
-                int err = request_module("bpfilter");
+        int err;
+        mutex_lock(&bpfilter_ops.lock);
+        if (!bpfilter_ops.sockopt) {
+                mutex_unlock(&bpfilter_ops.lock);
+                err = request_module("bpfilter");
+                mutex_lock(&bpfilter_ops.lock);
 
                 if (err)
-                        return err;
-                if (!bpfilter_process_sockopt)
-                        return -ECHILD;
+                        goto out;
+                if (!bpfilter_ops.sockopt) {
+                        err = -ECHILD;
+                        goto out;
+                }
+        }
+        if (bpfilter_ops.stop) {
+                err = bpfilter_ops.start();
+                if (err)
+                        goto out;
         }
-        return bpfilter_process_sockopt(sk, optname, optval, optlen, is_set);
+        err = bpfilter_ops.sockopt(sk, optname, optval, optlen, is_set);
+out:
+        mutex_unlock(&bpfilter_ops.lock);
+        return err;
 }
 
 int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char __user *optval,
@@ -41,3 +67,15 @@ int bpfilter_ip_get_sockopt(struct sock *sk, int optname, char __user *optval,
 
         return bpfilter_mbox_request(sk, optname, optval, len, false);
 }
+
+static int __init bpfilter_sockopt_init(void)
+{
+        mutex_init(&bpfilter_ops.lock);
+        bpfilter_ops.stop = true;
+        bpfilter_ops.info.cmdline = "bpfilter_umh";
+        bpfilter_ops.info.cleanup = &bpfilter_umh_cleanup;
+
+        return 0;
+}
+
+module_init(bpfilter_sockopt_init);