diff mbox series

Backport of 11cc2d21499c for 5.4.y and 4.19.y

Message ID 20210518112739.14086-1-magnus.karlsson@gmail.com
State New
Headers show
Series Backport of 11cc2d21499c for 5.4.y and 4.19.y | expand

Commit Message

Magnus Karlsson May 18, 2021, 11:27 a.m. UTC 
From: Magnus Karlsson <magnus.karlsson@intel.com>

Hi Greg and Sasha,

Please find attached backports of commit 11cc2d21499c ("xsk: Simplify
detection of empty and full rings") for the 5.4.y and 4.19.y stable
series. It solves a reproducible race between poll() and sendmsg()
when used concurrently by two different processes using the same
AF_XDP socket. Note that the commit above unknowingly (read: by sheer
luck) fixed the bug as it was about simplification and performance
improvement only. I have included two backports that are code wise
equivallent to the commit above, however, they contain a commit message
that describes the race in question and how it is fixed by the
patch. Sorry, but I do not know the correct procedure in these kind of
situations, but if you prefer to pick the original commit, please
ignore the "backports" below.

Please let me know if there are any problems.

Thanks: Magnus


>From aa84d8c8e0ba1e83a3454df04cd6bd417ee2bc8e Mon Sep 17 00:00:00 2001
From: Magnus Karlsson <magnus.karlsson@intel.com>
Date: Thu, 19 Dec 2019 13:39:21 +0100
Subject: [PATCH 4.19] xsk: fix race between poll and the driver

commit 11cc2d21499cabe7e7964389634ed1de3ee91d33 upstream

Fix a race between poll() and the driver that can result in one or
more packets being transmitted or received twice. The problem is that
poll() uses the same function the driver uses to access the Rx and Tx
rings in user space, and this function will update the state of one of
these rings under certain conditions. E.g., if the poll() call from
one process updates the Tx ring state while at the same time the
driver in zero-copy mode is processing entries in the ring (invoked by
sendmsg() or an interrupt), some packets will be sent twice. All the
AF_XDP rings are single producer / single consumer, so modifying one
from two places at the same time will corrupt it. Similarly, on the Rx
side packets might be received twice.

Fix this by changing the poll() function to not use the same function
as the driver uses and instead only read state from the ring.

Fixes: 35fcde7f8deb ("xsk: support for Tx")
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Benoit Ganne <bganne@cisco.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/BYAPR11MB365382C5DB1E5FCC53242609C1549@BYAPR11MB3653.namprd11.prod.outlook.com/
---
 net/xdp/xsk_queue.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


base-commit: b82e5721a17325739adef83a029340a63b53d4ad
--
2.29.0

Comments

Sasha Levin May 19, 2021, 3:12 p.m. UTC | #1 
On Tue, May 18, 2021 at 01:27:39PM +0200, Magnus Karlsson wrote:
>From: Magnus Karlsson <magnus.karlsson@intel.com>

>

>Hi Greg and Sasha,

>

>Please find attached backports of commit 11cc2d21499c ("xsk: Simplify

>detection of empty and full rings") for the 5.4.y and 4.19.y stable

>series. It solves a reproducible race between poll() and sendmsg()

>when used concurrently by two different processes using the same

>AF_XDP socket. Note that the commit above unknowingly (read: by sheer

>luck) fixed the bug as it was about simplification and performance

>improvement only. I have included two backports that are code wise

>equivallent to the commit above, however, they contain a commit message

>that describes the race in question and how it is fixed by the

>patch. Sorry, but I do not know the correct procedure in these kind of

>situations, but if you prefer to pick the original commit, please

>ignore the "backports" below.


In general, if it applies/builds/works cleanly, just telling us the
commit id would be enough. I've queued this one for 5.4 and 4.19,
thanks!

-- 
Thanks,
Sasha
diff mbox series

Patch

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index fe96c0d039f2..cf7cbb5dd918 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -245,12 +245,15 @@  static inline void xskq_produce_flush_desc(struct xsk_queue *q)

 static inline bool xskq_full_desc(struct xsk_queue *q)
 {
-	return xskq_nb_avail(q, q->nentries) == q->nentries;
+	/* No barriers needed since data is not accessed */
+	return READ_ONCE(q->ring->producer) - READ_ONCE(q->ring->consumer) ==
+		q->nentries;
 }

 static inline bool xskq_empty_desc(struct xsk_queue *q)
 {
-	return xskq_nb_free(q, q->prod_tail, q->nentries) == q->nentries;
+	/* No barriers needed since data is not accessed */
+	return READ_ONCE(q->ring->consumer) == READ_ONCE(q->ring->producer);
 }

 void xskq_set_umem(struct xsk_queue *q, struct xdp_umem_props *umem_props);

base-commit: 3c8c23092588a23bf1856a64f58c37f477a413be
--
2.29.0


>From 07f2ccb39bd20e90293c59ffcc977c14cf0ce577 Mon Sep 17 00:00:00 2001
From: Magnus Karlsson <magnus.karlsson@intel.com>
Date: Thu, 19 Dec 2019 13:39:21 +0100
Subject: [PATCH 5.4] xsk: fix race between poll and the driver

commit 11cc2d21499cabe7e7964389634ed1de3ee91d33 upstream

Fix a race between poll() and the driver that can result in one or
more packets being transmitted or received twice. The problem is that
poll() uses the same function the driver uses to access the Rx and Tx
rings in user space, and this function will update the state of one of
these rings under certain conditions. E.g., if the poll() call from
one process updates the Tx ring state while at the same time the
driver in zero-copy mode is processing entries in the ring (invoked by
sendmsg() or an interrupt), some packets will be sent twice. All the
AF_XDP rings are single producer / single consumer, so modifying one
from two places at the same time will corrupt it. Similarly, on the Rx
side packets might be received twice.

Fix this by changing the poll() function to not use the same function
as the driver uses and instead only read state from the ring.

Fixes: 35fcde7f8deb ("xsk: support for Tx")
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Benoit Ganne <bganne@cisco.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/BYAPR11MB365382C5DB1E5FCC53242609C1549@BYAPR11MB3653.namprd11.prod.outlook.com/
---
 net/xdp/xsk_queue.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index eddae4688862..ee3f8c857dd8 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -363,12 +363,15 @@  static inline void xskq_produce_flush_desc(struct xsk_queue *q)

 static inline bool xskq_full_desc(struct xsk_queue *q)
 {
-	return xskq_nb_avail(q, q->nentries) == q->nentries;
+	/* No barriers needed since data is not accessed */
+	return READ_ONCE(q->ring->producer) - READ_ONCE(q->ring->consumer) ==
+		q->nentries;
 }

 static inline bool xskq_empty_desc(struct xsk_queue *q)
 {
-	return xskq_nb_free(q, q->prod_tail, q->nentries) == q->nentries;
+	/* No barriers needed since data is not accessed */
+	return READ_ONCE(q->ring->consumer) == READ_ONCE(q->ring->producer);
 }

 void xskq_set_umem(struct xsk_queue *q, u64 size, u64 chunk_mask);