[PATCH net v2] pktgen: do not sleep with the thread lock held.

[PATCH net v2] pktgen: do not sleep with the thread lock held.

[Paolo Abeni]

Currently, the process issuing a "start" command on the pktgen procfs
interface, acquires the pktgen thread lock and never release it, until
all pktgen threads are completed. The above can blocks indefinitely any
other pktgen command and any (even unrelated) netdevice removal - as
the pktgen netdev notifier acquires the same lock.

The issue is demonstrated by the following script, reported by Matteo:

ip -b - <<'EOF'
	link add type dummy
	link add type veth
	link set dummy0 up
EOF
modprobe pktgen
echo reset >/proc/net/pktgen/pgctrl
{
	echo rem_device_all
	echo add_device dummy0
} >/proc/net/pktgen/kpktgend_0
echo count 0 >/proc/net/pktgen/dummy0
echo start >/proc/net/pktgen/pgctrl &
sleep 1
rmmod veth

Fix the above releasing the thread lock around the sleep call.

Additionally we must prevent racing with forcefull rmmod - as the
thread lock no more protects from them. Instead, acquire a self-reference
before waiting for any thread. As a side effect, running

rmmod pktgen

while some thread is running now fails with "module in use" error,
before this patch such command hanged indefinitely.

Note: the issue predates the commit reported in the fixes tag, but
this fix can't be applied before the mentioned commit.

v1 -> v2:
 - no need to check for thread existence after flipping the lock,
   pktgen threads are freed only at net exit time
 -

Fixes: 6146e6a43b35 ("[PKTGEN]: Removes thread_{un,}lock() macros.")
Reported-and-tested-by: Matteo Croce <mcroce@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/pktgen.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 319ad5490fb3..a33d03c05ed6 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -3066,7 +3066,13 @@ static int pktgen_wait_thread_run(struct pktgen_thread *t)
 {
 	while (thread_is_running(t)) {
 
+		/* note: 't' will still be around even after the unlock/lock
+		 * cycle because pktgen_thread threads are only cleared at
+		 * net exit
+		 */
+		mutex_unlock(&pktgen_thread_lock);
 		msleep_interruptible(100);
+		mutex_lock(&pktgen_thread_lock);
 
 		if (signal_pending(current))
 			goto signal;
@@ -3081,6 +3087,10 @@ static int pktgen_wait_all_threads_run(struct pktgen_net *pn)
 	struct pktgen_thread *t;
 	int sig = 1;
 
+	/* prevent from racing with rmmod */
+	if (!try_module_get(THIS_MODULE))
+		return sig;
+
 	mutex_lock(&pktgen_thread_lock);
 
 	list_for_each_entry(t, &pn->pktgen_threads, th_list) {
@@ -3094,6 +3104,7 @@ static int pktgen_wait_all_threads_run(struct pktgen_net *pn)
 			t->control |= (T_STOP);
 
 	mutex_unlock(&pktgen_thread_lock);
+	module_put(THIS_MODULE);
 	return sig;
 }
 
-- 
2.20.1

Re: [PATCH net v2] pktgen: do not sleep with the thread lock held.

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu,  6 Jun 2019 15:45:03 +0200

> Currently, the process issuing a "start" command on the pktgen procfs
> interface, acquires the pktgen thread lock and never release it, until
> all pktgen threads are completed. The above can blocks indefinitely any
> other pktgen command and any (even unrelated) netdevice removal - as
> the pktgen netdev notifier acquires the same lock.
> 
> The issue is demonstrated by the following script, reported by Matteo:
> 
> ip -b - <<'EOF'
> 	link add type dummy
> 	link add type veth
> 	link set dummy0 up
> EOF
> modprobe pktgen
> echo reset >/proc/net/pktgen/pgctrl
> {
> 	echo rem_device_all
> 	echo add_device dummy0
> } >/proc/net/pktgen/kpktgend_0
> echo count 0 >/proc/net/pktgen/dummy0
> echo start >/proc/net/pktgen/pgctrl &
> sleep 1
> rmmod veth
> 
> Fix the above releasing the thread lock around the sleep call.
> 
> Additionally we must prevent racing with forcefull rmmod - as the
> thread lock no more protects from them. Instead, acquire a self-reference
> before waiting for any thread. As a side effect, running
> 
> rmmod pktgen
> 
> while some thread is running now fails with "module in use" error,
> before this patch such command hanged indefinitely.
> 
> Note: the issue predates the commit reported in the fixes tag, but
> this fix can't be applied before the mentioned commit.
> 
> v1 -> v2:
>  - no need to check for thread existence after flipping the lock,
>    pktgen threads are freed only at net exit time
>  -
> 
> Fixes: 6146e6a43b35 ("[PKTGEN]: Removes thread_{un,}lock() macros.")
> Reported-and-tested-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

This looks a lot simpler.

Applied and queued up for -stable, thanks.