| Message ID | 150878036830.4768.8540758939081367484.stgit@firesoul |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] bpf: cpumap fix potential lost wake-up problem | expand |
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 ...
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?
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.
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();
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(-)