[net-next] bpf: cpumap fix potential lost wake-up problem

Message ID 150878036830.4768.8540758939081367484.stgit@firesoul
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net-next] bpf: cpumap fix potential lost wake-up problem
Related show

Commit Message

Jesper Dangaard Brouer Oct. 23, 2017, 5:39 p.m.
As pointed out by Michael, commit 1c601d829ab0 ("bpf: cpumap xdp_buff
to skb conversion and allocation") contains a classical example of the
potential lost wake-up problem.

We need to recheck the condition __ptr_ring_empty() after changing
current->state to TASK_INTERRUPTIBLE, this avoids a race between
wake_up_process() and schedule(). After this, a race with
wake_up_process() will simply change the state to TASK_RUNNING, and
the schedule() call not really put us to sleep.

Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
Reported-by: "Michael S. Tsirkin" <mst@redhat.com>
---
 kernel/bpf/cpumap.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Daniel Borkmann Oct. 23, 2017, 8:34 p.m. | #1
On 10/23/2017 07:39 PM, Jesper Dangaard Brouer wrote:
> As pointed out by Michael, commit 1c601d829ab0 ("bpf: cpumap xdp_buff
> to skb conversion and allocation") contains a classical example of the
> potential lost wake-up problem.
>
> We need to recheck the condition __ptr_ring_empty() after changing
> current->state to TASK_INTERRUPTIBLE, this avoids a race between
> wake_up_process() and schedule(). After this, a race with
> wake_up_process() will simply change the state to TASK_RUNNING, and
> the schedule() call not really put us to sleep.
>
> Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
> Reported-by: "Michael S. Tsirkin" <mst@redhat.com>

SOB missing ...
Jesper Dangaard Brouer Oct. 24, 2017, 4:46 a.m. | #2
On Mon, 23 Oct 2017 22:34:37 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 10/23/2017 07:39 PM, Jesper Dangaard Brouer wrote:
> > As pointed out by Michael, commit 1c601d829ab0 ("bpf: cpumap xdp_buff
> > to skb conversion and allocation") contains a classical example of the
> > potential lost wake-up problem.
> >
> > We need to recheck the condition __ptr_ring_empty() after changing
> > current->state to TASK_INTERRUPTIBLE, this avoids a race between
> > wake_up_process() and schedule(). After this, a race with
> > wake_up_process() will simply change the state to TASK_RUNNING, and
> > the schedule() call not really put us to sleep.
> >
> > Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
> > Reported-by: "Michael S. Tsirkin" <mst@redhat.com>  
> 
> SOB missing ...

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Damn... DaveM do I need to resubmit? Or will patchwork pickup above SOB?
David Miller Oct. 24, 2017, 9:40 a.m. | #3
From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Mon, 23 Oct 2017 19:39:28 +0200

> As pointed out by Michael, commit 1c601d829ab0 ("bpf: cpumap xdp_buff
> to skb conversion and allocation") contains a classical example of the
> potential lost wake-up problem.
> 
> We need to recheck the condition __ptr_ring_empty() after changing
> current->state to TASK_INTERRUPTIBLE, this avoids a race between
> wake_up_process() and schedule(). After this, a race with
> wake_up_process() will simply change the state to TASK_RUNNING, and
> the schedule() call not really put us to sleep.
> 
> Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
> Reported-by: "Michael S. Tsirkin" <mst@redhat.com>

Applied.

Patch

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index b4358d84ddf1..86e29cbf7827 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -288,13 +288,17 @@  static int cpu_map_kthread_run(void *data)
 
 		/* Release CPU reschedule checks */
 		if (__ptr_ring_empty(rcpu->queue)) {
-			__set_current_state(TASK_INTERRUPTIBLE);
-			schedule();
-			sched = 1;
+			set_current_state(TASK_INTERRUPTIBLE);
+			/* Recheck to avoid lost wake-up */
+			if (__ptr_ring_empty(rcpu->queue)) {
+				schedule();
+				sched = 1;
+			} else {
+				__set_current_state(TASK_RUNNING);
+			}
 		} else {
 			sched = cond_resched();
 		}
-		__set_current_state(TASK_RUNNING);
 
 		/* Process packets in rcpu->queue */
 		local_bh_disable();