[PATCH bpf 0/2] xsk: fix two bugs in the SKB Tx path

[PATCH bpf 0/2] xsk: fix two bugs in the SKB Tx path

[Magnus Karlsson]

This patch set contains two bug fixes to the Tx SKB path. Details can
be found in the individual commit messages. Special thanks to Xuan
Zhuo for spotting both of them.

Thanks: Magnus

Magnus Karlsson (2):
  xsk: fix race in SKB mode transmit with shared cq
  xsk: rollback reservation at NETDEV_TX_BUSY

 include/net/xdp_sock.h      |  4 ----
 include/net/xsk_buff_pool.h |  5 +++++
 net/xdp/xsk.c               | 12 +++++++++---
 net/xdp/xsk_buff_pool.c     |  1 +
 net/xdp/xsk_queue.h         |  5 +++++
 5 files changed, 20 insertions(+), 7 deletions(-)


base-commit: d9838b1d39283c1200c13f9076474c7624b8ec34
--
2.29.0

[PATCH bpf 1/2] xsk: fix race in SKB mode transmit with shared cq

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix a race when multiple sockets are simultaneously calling sendto()
when the completion ring is shared in the SKB case. This is the case
when you share the same netdev and queue id through the
XDP_SHARED_UMEM bind flag. The problem is that multiple processes can
be in xsk_generic_xmit() and call the backpressure mechanism in
xskq_prod_reserve(xs->pool->cq). As this is a shared resource in this
specific scenario, a race might occur since the rings are
single-producer single-consumer.

Fix this by moving the tx_completion_lock from the socket to the pool
as the pool is shared between the sockets that share the completion
ring. (The pool is not shared when this is not the case.) And then
protect the accesses to xskq_prod_reserve() with this lock. The
tx_completion_lock is renamed cq_lock to better reflect that it
protects accesses to the potentially shared completion ring.

Fixes: 35fcde7f8deb ("xsk: support for Tx")
Fixes: a9744f7ca200 ("xsk: fix potential race in SKB TX completion code")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/net/xdp_sock.h      | 4 ----
 include/net/xsk_buff_pool.h | 5 +++++
 net/xdp/xsk.c               | 9 ++++++---
 net/xdp/xsk_buff_pool.c     | 1 +
 4 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 4f4e93bf814c..cc17bc957548 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -58,10 +58,6 @@ struct xdp_sock {
 
 	struct xsk_queue *tx ____cacheline_aligned_in_smp;
 	struct list_head tx_list;
-	/* Mutual exclusion of NAPI TX thread and sendmsg error paths
-	 * in the SKB destructor callback.
-	 */
-	spinlock_t tx_completion_lock;
 	/* Protects generic receive. */
 	spinlock_t rx_lock;
 
diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index 01755b838c74..eaa8386dbc63 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -73,6 +73,11 @@ struct xsk_buff_pool {
 	bool dma_need_sync;
 	bool unaligned;
 	void *addrs;
+	/* Mutual exclusion of the completion ring in the SKB mode. Two cases to protect:
+	 * NAPI TX thread and sendmsg error paths in the SKB destructor callback and when
+	 * sockets share a single cq when the same netdev and queue id is shared.
+	 */
+	spinlock_t cq_lock;
 	struct xdp_buff_xsk *free_heads[];
 };
 
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 62504471fd20..42cb5f94d49e 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -364,9 +364,9 @@ static void xsk_destruct_skb(struct sk_buff *skb)
 	struct xdp_sock *xs = xdp_sk(skb->sk);
 	unsigned long flags;
 
-	spin_lock_irqsave(&xs->tx_completion_lock, flags);
+	spin_lock_irqsave(&xs->pool->cq_lock, flags);
 	xskq_prod_submit_addr(xs->pool->cq, addr);
-	spin_unlock_irqrestore(&xs->tx_completion_lock, flags);
+	spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
 
 	sock_wfree(skb);
 }
@@ -378,6 +378,7 @@ static int xsk_generic_xmit(struct sock *sk)
 	bool sent_frame = false;
 	struct xdp_desc desc;
 	struct sk_buff *skb;
+	unsigned long flags;
 	int err = 0;
 
 	mutex_lock(&xs->mutex);
@@ -409,10 +410,13 @@ static int xsk_generic_xmit(struct sock *sk)
 		 * if there is space in it. This avoids having to implement
 		 * any buffering in the Tx path.
 		 */
+		spin_lock_irqsave(&xs->pool->cq_lock, flags);
 		if (unlikely(err) || xskq_prod_reserve(xs->pool->cq)) {
+			spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
 			kfree_skb(skb);
 			goto out;
 		}
+		spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
 
 		skb->dev = xs->dev;
 		skb->priority = sk->sk_priority;
@@ -1193,7 +1197,6 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
 	xs->state = XSK_READY;
 	mutex_init(&xs->mutex);
 	spin_lock_init(&xs->rx_lock);
-	spin_lock_init(&xs->tx_completion_lock);
 
 	INIT_LIST_HEAD(&xs->map_list);
 	spin_lock_init(&xs->map_list_lock);
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index d5adeee9d5d9..7da28566ac11 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -71,6 +71,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 	INIT_LIST_HEAD(&pool->free_list);
 	INIT_LIST_HEAD(&pool->xsk_tx_list);
 	spin_lock_init(&pool->xsk_tx_list_lock);
+	spin_lock_init(&pool->cq_lock);
 	refcount_set(&pool->users, 1);
 
 	pool->fq = xs->fq_tmp;
-- 
2.29.0

[PATCH bpf 2/2] xsk: rollback reservation at NETDEV_TX_BUSY

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Rollback the reservation in the completion ring when we get a
NETDEV_TX_BUSY. When this error is received from the driver, we are
supposed to let the user application retry the transmit again. And in
order to do this, we need to roll back the failed send so it can be
retried. Unfortunately, we did not cancel the reservation we had made
in the completion ring. By not doing this, we actually make the
completion ring one entry smaller per NETDEV_TX_BUSY error we get, and
after enough of these errors the completion ring will be of size zero
and transmit will stop working.

Fix this by cancelling the reservation when we get a NETDEV_TX_BUSY
error.

Fixes: 642e450b6b59 ("xsk: Do not discard packet when NETDEV_TX_BUSY")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 net/xdp/xsk.c       | 3 +++
 net/xdp/xsk_queue.h | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 42cb5f94d49e..2587583a6be9 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -428,6 +428,9 @@ static int xsk_generic_xmit(struct sock *sk)
 		if  (err == NETDEV_TX_BUSY) {
 			/* Tell user-space to retry the send */
 			skb->destructor = sock_wfree;
+			spin_lock_irqsave(&xs->pool->cq_lock, flags);
+			xskq_prod_cancel(xs->pool->cq);
+			spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
 			/* Free skb without triggering the perf drop trace */
 			consume_skb(skb);
 			err = -EAGAIN;
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 9e71b9f27679..ef6de0fb4e31 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -286,6 +286,11 @@ static inline bool xskq_prod_is_full(struct xsk_queue *q)
 	return !free_entries;
 }
 
+static inline void xskq_prod_cancel(struct xsk_queue *q)
+{
+	q->cached_prod--;
+}
+
 static inline int xskq_prod_reserve(struct xsk_queue *q)
 {
 	if (xskq_prod_is_full(q))
-- 
2.29.0

Re: [PATCH bpf 0/2] xsk: fix two bugs in the SKB Tx path

[Björn Töpel]

On Mon, 14 Dec 2020 at 16:35, Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
>
> This patch set contains two bug fixes to the Tx SKB path. Details can
> be found in the individual commit messages. Special thanks to Xuan
> Zhuo for spotting both of them.
>
> Thanks: Magnus
>
> Magnus Karlsson (2):
>   xsk: fix race in SKB mode transmit with shared cq
>   xsk: rollback reservation at NETDEV_TX_BUSY
>

For the series:

Acked-by: Björn Töpel <bjorn.topel@intel.com>

>  include/net/xdp_sock.h      |  4 ----
>  include/net/xsk_buff_pool.h |  5 +++++
>  net/xdp/xsk.c               | 12 +++++++++---
>  net/xdp/xsk_buff_pool.c     |  1 +
>  net/xdp/xsk_queue.h         |  5 +++++
>  5 files changed, 20 insertions(+), 7 deletions(-)
>
>
> base-commit: d9838b1d39283c1200c13f9076474c7624b8ec34
> --
> 2.29.0