Skip to contentSkip to conversation

net: bpfilter: use get_pid_task instead of pid_task

PATCH In review Taehee Yoo 3 participants 3 messages Oct 16, 2018 – Oct 18, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Taehee Yoo

author
#0 lore raw

PATCH 1/1 net: bpfilter: use get_pid_task instead of pid_task

pid_task() dereferences rcu protected tasks array.
But there is no rcu_read_lock() in shutdown_umh() routine so that rcu_read_lock() is needed.
get_pid_task() is wrapper function of pid_task. it holds rcu_read_lock() then calls pid_task(). if task isn't NULL, it increases reference count of task.

test commands:
%modprobe bpfilter
%modprobe -rv bpfilter

splat looks like:

[15102.030932] =============================
[15102.030957] WARNING: suspicious RCU usage
[15102.030985] 4.19.0-rc7+ #21 Not tainted
[15102.031010] -----------------------------
[15102.031038] kernel/pid.c:330 suspicious rcu_dereference_check() usage!
[15102.031063]
	       other info that might help us debug this:
[15102.031332]
	       rcu_scheduler_active = 2, debug_locks = 1
[15102.031363] 1 lock held by modprobe/1570:
[15102.031389]  #0: 00000000580ef2b0 (bpfilter_lock){+.+.}, at: stop_umh+0x13/0x52 [bpfilter]
[15102.031552]
               stack backtrace:
[15102.031583] CPU: 1 PID: 1570 Comm: modprobe Not tainted 4.19.0-rc7+ #21
[15102.031607] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 07/08/2015
[15102.031628] Call Trace:
[15102.031676]  dump_stack+0xc9/0x16b
[15102.031723]  ? show_regs_print_info+0x5/0x5
[15102.031801]  ? lockdep_rcu_suspicious+0x117/0x160
[15102.031855]  pid_task+0x134/0x160
[15102.031900]  ? find_vpid+0xf0/0xf0
[15102.032017]  shutdown_umh.constprop.1+0x1e/0x53 [bpfilter]
[15102.032055]  stop_umh+0x46/0x52 [bpfilter]
[15102.032092]  __x64_sys_delete_module+0x47e/0x570

[ ... ]

Fixes d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
Signed-off-by Taehee Yoo <[email protected]>
---
 net/bpfilter/bpfilter_kern.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
net/bpfilter/bpfilter_kern.c +4 -2
index b64e1649993b..94e88f510c5b 100644
@@ -23,9 +23,11 @@ static void shutdown_umh(struct umh_info *info)
 
 	if (!info->pid)
 		return;
-	tsk = pid_task(find_vpid(info->pid), PIDTYPE_PID);
-	if (tsk)
+	tsk = get_pid_task(find_vpid(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;
Signature · 1 line
2.17.1
Acked-by on v1 1/1
#1 lore raw
Quoted 37 lines · Taehee Yoo

pid_task() dereferences rcu protected tasks array.
But there is no rcu_read_lock() in shutdown_umh() routine so that rcu_read_lock() is needed.
get_pid_task() is wrapper function of pid_task. it holds rcu_read_lock() then calls pid_task(). if task isn't NULL, it increases reference count of task.

test commands:
%modprobe bpfilter
%modprobe -rv bpfilter

splat looks like:

[15102.030932] =============================
[15102.030957] WARNING: suspicious RCU usage
[15102.030985] 4.19.0-rc7+ #21 Not tainted
[15102.031010] -----------------------------
[15102.031038] kernel/pid.c:330 suspicious rcu_dereference_check() usage!
[15102.031063]
	       other info that might help us debug this:
[15102.031332]
	       rcu_scheduler_active = 2, debug_locks = 1
[15102.031363] 1 lock held by modprobe/1570:
[15102.031389]  #0: 00000000580ef2b0 (bpfilter_lock){+.+.}, at: stop_umh+0x13/0x52 [bpfilter]
[15102.031552]
               stack backtrace:
[15102.031583] CPU: 1 PID: 1570 Comm: modprobe Not tainted 4.19.0-rc7+ #21
[15102.031607] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 07/08/2015
[15102.031628] Call Trace:
[15102.031676]  dump_stack+0xc9/0x16b
[15102.031723]  ? show_regs_print_info+0x5/0x5
[15102.031801]  ? lockdep_rcu_suspicious+0x117/0x160
[15102.031855]  pid_task+0x134/0x160
[15102.031900]  ? find_vpid+0xf0/0xf0
[15102.032017]  shutdown_umh.constprop.1+0x1e/0x53 [bpfilter]
[15102.032055]  stop_umh+0x46/0x52 [bpfilter]
[15102.032092]  __x64_sys_delete_module+0x47e/0x570
Quoted 4 lines

[ ... ]

Fixes d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
Signed-off-by Taehee Yoo <[email protected]>

thanks a lot for the fix

Acked-by Alexei Starovoitov <[email protected]>
maintainer
#2 lore raw
In reply to Taehee Yoo

From: Taehee Yoo <[email protected]>
Date: Wed, 17 Oct 2018 00:35:10 +0900

Quoted 10 lines

pid_task() dereferences rcu protected tasks array.
But there is no rcu_read_lock() in shutdown_umh() routine so that rcu_read_lock() is needed.
get_pid_task() is wrapper function of pid_task. it holds rcu_read_lock() then calls pid_task(). if task isn't NULL, it increases reference count of task.

test commands:
%modprobe bpfilter
%modprobe -rv bpfilter

...

Quoted 2 lines
Fixes d2ba09c17a06 ("net: add skeleton of bpfilter kernel module")
Signed-off-by Taehee Yoo <[email protected]>

Applied and queued up for -stable, thanks.