virtio_net: fix refill related races

Fix theoretical races related to refill work:
1. After napi is disabled by ndo_stop, refill work can run and re-enable it.
2. Refill can reschedule itself, if this happens it can run after cancel_delayed_work_sync, and will access device after it is destroyed.

As a solution, add flags to track napi state and
to disable refill, and toggle them on start, stop
and remove; check these flags on refill.

Quoted 7 lines · Michael S. Tsirkin

Fix theoretical races related to refill work:
1. After napi is disabled by ndo_stop, refill work can run and re-enable it.
2. Refill can reschedule itself, if this happens it can run after cancel_delayed_work_sync, and will access device after it is destroyed.

Quoted 3 lines

As a solution, add flags to track napi state and
to disable refill, and toggle them on start, stop
and remove; check these flags on refill.

Why isn't a "dont-readd" flag sufficient?

Cheers,
Rusty.

Quoted 8 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

Fix theoretical races related to refill work:
1. After napi is disabled by ndo_stop, refill work can run and re-enable it.
2. Refill can reschedule itself, if this happens it can run after cancel_delayed_work_sync, and will access device after it is destroyed.

Quoted 8 lines

Quoted 3 lines

As a solution, add flags to track napi state and
to disable refill, and toggle them on start, stop
and remove; check these flags on refill.

Why isn't a "dont-readd" flag sufficient?

Cheers,
Rusty.

I started with that, but here's the problem I wanted to address:

- we run out of descriptors and schedule refill work
- ndo_close runs
- refill work runs
- ndo_open runs

Quoted 18 lines · Michael S. Tsirkin

Quoted 8 lines · Rusty Russell

Quoted 7 lines · Michael S. Tsirkin

Fix theoretical races related to refill work:
1. After napi is disabled by ndo_stop, refill work can run and re-enable it.
2. Refill can reschedule itself, if this happens it can run after cancel_delayed_work_sync, and will access device after it is destroyed.

Quoted 8 lines

Quoted 3 lines

As a solution, add flags to track napi state and
to disable refill, and toggle them on start, stop
and remove; check these flags on refill.

Why isn't a "dont-readd" flag sufficient?

Cheers,
Rusty.

Quoted 7 lines

I started with that, but here's the problem I wanted to address:

- we run out of descriptors and schedule refill work
- ndo_close runs
- refill work runs
- ndo_open runs

(s/ndo_close/ndo_stop/)

You don't think we should do any refills on a closed device? If so, we simply move the refill-stop code into ndo_stop (aka virtnet_close), and the refill-start code into ndo_open (aka. virtnet_open). Right?

Orthogonally, the refill-stop code is still buggy, as you noted.

And
for self-rearming timers the pattern I've always used is a flag.

Or am I being obtuse again? :)
Rusty.

Quoted 1 line · Rusty Russell

Orthogonally, the refill-stop code is still buggy, as you noted.

Sorry I don't understand how it's still buggy.

Both places where we call:

	cancel_delayed_work_sync(&vi->refill);

Do not actually guarantee that vi->refill isn't running, because it can requeue itself. A 'bool no_more_refill' field seems like the simplest fix for this, but I don't think it's sufficient.

Tejun, is this correct? What's the correct way to synchronously stop a delayed_work which can "schedule_delayed_work(&vi->refill, HZ/2);" on itself?

Quoted 4 lines · Michael S. Tsirkin

Quoted 1 line · Rusty Russell

Orthogonally, the refill-stop code is still buggy, as you noted.

Sorry I don't understand how it's still buggy.

Both places where we call:

	cancel_delayed_work_sync(&vi->refill);

Do not actually guarantee that vi->refill isn't running, because it can requeue itself. A 'bool no_more_refill' field seems like the simplest fix for this, but I don't think it's sufficient.

Tejun, is this correct? What's the correct way to synchronously stop a delayed_work which can "schedule_delayed_work(&vi->refill, HZ/2);" on itself?

Thanks,
Rusty.

Another question, wanted to make sure:
virtnet_poll does schedule_delayed_work(&vi->refill, 0); separately refill work itself also does schedule_delayed_work(&vi->refill, HZ/2);
If two such events happen twice, on different CPUs, we are still guaranteed the work will only run once, right?

Hello, Michael.

Quoted 6 lines · Michael S. Tsirkin

Another question, wanted to make sure:
virtnet_poll does schedule_delayed_work(&vi->refill, 0); separately refill work itself also does schedule_delayed_work(&vi->refill, HZ/2);
If two such events happen twice, on different CPUs, we are still guaranteed the work will only run once, right?

No, it's not. Normal workqueues only guarantee non-reentrance on local CPU. If you want to guarantee that only one instance of a given item is executing across all CPUs, you need to use the nrt workqueue.

Thanks.

Signature · 1 line

tejun

Hmm, in that case it looks like a nasty race could get triggered, with try_fill_recv run on multiple CPUs in parallel, corrupting the linked list within the vq.

Using the mutex as my patch did will fix that naturally, as well.

Quoted 5 lines · Michael S. Tsirkin

Hmm, in that case it looks like a nasty race could get triggered, with try_fill_recv run on multiple CPUs in parallel, corrupting the linked list within the vq.

Using the mutex as my patch did will fix that naturally, as well.

Don't know the code but just use nrt wq. There's even a system one called system_nrq_wq.

Thanks.

Signature · 1 line

tejun

Quoted 5 lines · Tejun Heo

Quoted 4 lines · Michael S. Tsirkin

Hmm, in that case it looks like a nasty race could get triggered, with try_fill_recv run on multiple CPUs in parallel, corrupting the linked list within the vq.

Quoted 6 lines

Quoted 1 line

Using the mutex as my patch did will fix that naturally, as well.

Don't know the code but just use nrt wq. There's even a system one called system_nrq_wq.

Thanks.

We can, but we need the mutex for other reasons, anyway.

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Tejun Heo

Quoted 5 lines · Michael S. Tsirkin

Hmm, in that case it looks like a nasty race could get triggered, with try_fill_recv run on multiple CPUs in parallel, corrupting the linked list within the vq.

Using the mutex as my patch did will fix that naturally, as well.

Quoted 6 lines

Quoted 4 lines

Don't know the code but just use nrt wq. There's even a system one called system_nrq_wq.

Thanks.

We can, but we need the mutex for other reasons, anyway.

Well, here's the alternate approach. What do you think?

Finding two wq issues makes you justifiably cautious, but it almost feels like giving up to simply wrap it in a lock. The APIs are designed to let us do it without a lock; I was just using them wrong.

Two patches in one mail. It's gauche, but it's an RFC only (untested).

Cheers,
Rusty.

virtio_net: set/cancel work on ndo_open/ndo_stop

Michael S. Tsirkin noticed that we could run the refill work after ndo_close, which can re-enable napi - we don't disable it until virtnet_remove. This is clearly wrong, so move the workqueue control to ndo_open and ndo_stop (aka. virtnet_open and virtnet_close).

One subtle point: virtnet_probe() could simply fail if it couldn't allocate a receive buffer, but that's less polite in virtnet_open() so we schedule a refill as we do in the normal receive path if we run out of memory.

drivers/net/virtio_net.c +5 -3

+	if (!try_fill_recv(vi, GFP_KERNEL))
+		schedule_delayed_work(&vi->refill, 0);
+
 	virtnet_napi_enable(vi);
 	return 0;
 }

@@ -772,6 +776,8 @@ static int virtnet_close(struct net_devi
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 
+	/* Make sure refill_work doesn't re-enable napi! */
+	cancel_delayed_work_sync(&vi->refill);
 	napi_disable(&vi->napi);
 
 	return 0;

@@ -1082,7 +1088,6 @@ static int virtnet_probe(struct virtio_d
 
 unregister:
 	unregister_netdev(dev);
-	cancel_delayed_work_sync(&vi->refill);
 free_vqs:
 	vdev->config->del_vqs(vdev);
 free_stats:

@@ -1121,9 +1126,7 @@ static void __devexit virtnet_remove(str
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
 
-
 	unregister_netdev(vi->dev);
-	cancel_delayed_work_sync(&vi->refill);
 
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);

virtio_net: use non-reentrant workqueue.

Michael S. Tsirkin also noticed that we could run the refill work multiple CPUs: if we kick off a refill on one CPU and then on another, they would both manipulate the queue at the same time (they use napi_disable to avoid racing against the receive handler itself).

Tejun points out that this is what the WQ_NON_REENTRANT flag is for, and that there is a convenient system kthread we can use.

drivers/net/virtio_net.c +2 -2

@@ -501,7 +501,7 @@ static void refill_work(struct work_stru
 	/* In theory, this can happen: if we don't get any buffers in
 	 * we will *never* try to fill again. */
 	if (still_empty)
-		schedule_delayed_work(&vi->refill, HZ/2);
+		queue_delayed_work(system_nrt_wq, &vi->refill, HZ/2);
 }
 
 static int virtnet_poll(struct napi_struct *napi, int budget)

@@ -520,7 +520,7 @@ again:
 
 	if (vi->num < vi->max / 2) {
 		if (!try_fill_recv(vi, GFP_ATOMIC))
-			schedule_delayed_work(&vi->refill, 0);
+			queue_delayed_work(system_nrt_wq, &vi->refill, 0);
 	}
 
 	/* Out of packets? */

Quoted 12 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Tejun Heo

Quoted 5 lines · Michael S. Tsirkin

Hmm, in that case it looks like a nasty race could get triggered, with try_fill_recv run on multiple CPUs in parallel, corrupting the linked list within the vq.

Using the mutex as my patch did will fix that naturally, as well.

Don't know the code but just use nrt wq. There's even a system one called system_nrq_wq.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Thanks.

We can, but we need the mutex for other reasons, anyway.

Well, here's the alternate approach. What do you think?

It looks very clean, thanks. Some documentation suggestions below. Also - Cc stable on this and the block patch?

Quoted 3 lines

Finding two wq issues makes you justifiably cautious, but it almost feels like giving up to simply wrap it in a lock. The APIs are designed to let us do it without a lock; I was just using them wrong.

One thing I note is that this scheme works because there's a single entity disabling/enabling napi and the refill thread.
So it's possible that Amit will need to add a lock and track NAPI state anyway to make suspend work. But we'll see.