* [net-next PATCH] bpf: cpumap fix potential lost wake-up problem
@ 2017-10-23 17:39 Jesper Dangaard Brouer
2017-10-23 20:34 ` Daniel Borkmann
2017-10-24 9:40 ` David Miller
0 siblings, 2 replies; 4+ messages in thread
From: Jesper Dangaard Brouer @ 2017-10-23 17:39 UTC (permalink / raw)
To: netdev, Michael S. Tsirkin
Cc: Rik van Riel, Daniel Borkmann, Alexei Starovoitov,
Jesper Dangaard Brouer
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(-)
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();
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [net-next PATCH] bpf: cpumap fix potential lost wake-up problem
2017-10-23 17:39 [net-next PATCH] bpf: cpumap fix potential lost wake-up problem Jesper Dangaard Brouer
@ 2017-10-23 20:34 ` Daniel Borkmann
2017-10-24 4:46 ` Jesper Dangaard Brouer
2017-10-24 9:40 ` David Miller
1 sibling, 1 reply; 4+ messages in thread
From: Daniel Borkmann @ 2017-10-23 20:34 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, Michael S. Tsirkin
Cc: Rik van Riel, Daniel Borkmann, Alexei Starovoitov
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 ...
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [net-next PATCH] bpf: cpumap fix potential lost wake-up problem
2017-10-23 20:34 ` Daniel Borkmann
@ 2017-10-24 4:46 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 4+ messages in thread
From: Jesper Dangaard Brouer @ 2017-10-24 4:46 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, Michael S. Tsirkin, Rik van Riel, Daniel Borkmann,
Alexei Starovoitov, brouer
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?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [net-next PATCH] bpf: cpumap fix potential lost wake-up problem
2017-10-23 17:39 [net-next PATCH] bpf: cpumap fix potential lost wake-up problem Jesper Dangaard Brouer
2017-10-23 20:34 ` Daniel Borkmann
@ 2017-10-24 9:40 ` David Miller
1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2017-10-24 9:40 UTC (permalink / raw)
To: brouer; +Cc: netdev, mst, riel, borkmann, alexei.starovoitov
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-24 9:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-23 17:39 [net-next PATCH] bpf: cpumap fix potential lost wake-up problem Jesper Dangaard Brouer
2017-10-23 20:34 ` Daniel Borkmann
2017-10-24 4:46 ` Jesper Dangaard Brouer
2017-10-24 9:40 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).