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

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.
After re-acquiring the lock we must check again for the relevant
thread existence, as some other pktgen command could have
terminated it meanwhile.

In the caller, we can't continue walking the threads list, for the
same reason. Instead, let's pick the first running thread 'till we
can find any of them.

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

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

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 319ad5490fb3..09ce399986e2 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c

@@ -3062,20 +3062,49 @@ static int thread_is_running(const struct pktgen_thread *t)
 	return 0;
 }
 
-static int pktgen_wait_thread_run(struct pktgen_thread *t)
+static bool pktgen_lookup_thread(struct pktgen_net *pn, struct pktgen_thread *t)
+{
+	struct pktgen_thread *tmp;
+
+	list_for_each_entry(tmp, &pn->pktgen_threads, th_list)
+		if (tmp == t)
+			return true;
+	return false;
+}
+
+static int pktgen_wait_thread_run(struct pktgen_net *pn,
+				  struct pktgen_thread *t)
 {
 	while (thread_is_running(t)) {
 
+		mutex_unlock(&pktgen_thread_lock);
 		msleep_interruptible(100);
+		mutex_lock(&pktgen_thread_lock);
 
 		if (signal_pending(current))
 			goto signal;
+
+		/* in the meanwhile 't' can be dead, removed, etc, check again
+		 * its existence
+		 */
+		if (!pktgen_lookup_thread(pn, t))
+			break;
 	}
 	return 1;
 signal:
 	return 0;
 }
 
+static struct pktgen_thread *pktgen_find_first_running(struct pktgen_net *pn)
+{
+	struct pktgen_thread *t;
+
+	list_for_each_entry(t, &pn->pktgen_threads, th_list)
+		if (thread_is_running(t))
+			return t;
+	return NULL;
+}
+
 static int pktgen_wait_all_threads_run(struct pktgen_net *pn)
 {
 	struct pktgen_thread *t;

@@ -3083,8 +3112,11 @@ static int pktgen_wait_all_threads_run(struct pktgen_net *pn)
 
 	mutex_lock(&pktgen_thread_lock);
 
-	list_for_each_entry(t, &pn->pktgen_threads, th_list) {
-		sig = pktgen_wait_thread_run(t);
+	while (1) {
+		t = pktgen_find_first_running(pn);
+		if (!t)
+			break;
+		sig = pktgen_wait_thread_run(pn, t);
 		if (sig == 0)
 			break;
 	}

-- 
2.20.1

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed,  5 Jun 2019 14:34:46 +0200

Pointer equality is not object equality.

It is possible for a pktgen thread to be terminated, a new one started,
and the new one to have the same pointer value as the old one.

Hi,

Thank you for the feedback.

On Wed, 2019-06-05 at 12:05 -0700, David Miller wrote:

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed,  5 Jun 2019 14:34:46 +0200

quoted

@@ -3062,20 +3062,49 @@ static int thread_is_running(const struct pktgen_thread *t)
      return 0;
 }
 
-static int pktgen_wait_thread_run(struct pktgen_thread *t)
+static bool pktgen_lookup_thread(struct pktgen_net *pn, struct pktgen_thread *t)
+{
+     struct pktgen_thread *tmp;
+
+     list_for_each_entry(tmp, &pn->pktgen_threads, th_list)
+             if (tmp == t)
+                     return true;
+     return false;
+}

Pointer equality is not object equality.

Indeed. The trick is that pktgen threads are allocated only at net
creation time.

Actaully they are also freed only net exit, so the extra care is not
needed: the current process held a reference to net via fs proxy.

I'll send a v2 cleaning up the unneeded parts and some documenting
comments.

Cheers,

Paolo