netdev - [PATCH bpf] xsk: add missing memory barrier in xskq_has

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Nov 2019 10:51:10 +0100
From:   Magnus Karlsson <magnus.karlsson@...el.com>
To:     magnus.karlsson@...el.com, bjorn.topel@...el.com, ast@...nel.org,
        daniel@...earbox.net, netdev@...r.kernel.org,
        jonathan.lemon@...il.com
Cc:     maximmi@...lanox.com, bpf@...r.kernel.org
Subject: [PATCH bpf] xsk: add missing memory barrier in xskq_has_addrs()

The rings in AF_XDP between user space and kernel space have the
following semantics:

producer                         consumer

if (LOAD ->consumer) {           LOAD ->producer
                   (A)           smp_rmb()       (C)
   STORE $data                   LOAD $data
   smp_wmb()       (B)           smp_mb()        (D)
   STORE ->producer              STORE ->consumer
}

The consumer function xskq_has_addrs() below loads the producer
pointer and updates the locally cached copy of it. However, it does
not issue the smp_rmb() operation required by the lockless ring. This
would have been ok had the function not updated the locally cached
copy, as that could not have resulted in new data being read from the
ring. But as it updates the local producer pointer, a subsequent peek
operation, such as xskq_peek_addr(), might load data from the ring
without issuing the required smp_rmb() memory barrier.

static inline bool xskq_has_addrs(struct xsk_queue *q, u32 cnt)
{
        u32 entries = q->prod_tail - q->cons_tail;

        if (entries >= cnt)
                return true;

        /* Refresh the local pointer. */
        q->prod_tail = READ_ONCE(q->ring->producer);
	*** MISSING MEMORY BARRIER ***
        entries = q->prod_tail - q->cons_tail;

        return entries >= cnt;
}

Fix this by adding the missing memory barrier at the indicated point
above.

Fixes: d57d76428ae9 ("Add API to check for available entries in FQ")
Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
---
 net/xdp/xsk_queue.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index eddae46..b5492c3 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -127,6 +127,7 @@ static inline bool xskq_has_addrs(struct xsk_queue *q, u32 cnt)
 
 	/* Refresh the local pointer. */
 	q->prod_tail = READ_ONCE(q->ring->producer);
+	smp_rmb(); /* C, matches B */
 	entries = q->prod_tail - q->cons_tail;
 
 	return entries >= cnt;
-- 
2.7.4