[PATCH] xsk: Fix race condition in AF_XDP generic RX path

[PATCH] xsk: Fix race condition in AF_XDP generic RX path

[e.kubanski]

rx_lock moved from xsk_socket to xsk_buff_pool.
Previous synchronization didn't take care of
shared umem mode in generic RX path where sockets
share the same xsk_buff_pool.

RX queue is exclusive to xsk_socket, while FILL
queue can be shared between multiple sockets.
This could result in race condition where two
CPU cores access RX path of two different sockets
sharing the same umem.

Now both queues are protected by acquiring spinlock
in shared xsk_buff_pool.

Lock contention may be minimized in the future by some
per-thread FQ buffering.

It's safe and necessary to move spin_lock_bh(rx_lock)
after xsk_rcv_check():
* xs->pool and spinlock_init is synchronized by
  xsk_bind() -> xsk_is_bound() memory barriers.
* xsk_rcv_check() may return true at the moment
  of xsk_release() or xsk_unbind_dev(),
  however this will not cause any data races or
  race conditions. xsk_unbind_dev() removes xdp
  socket from all maps and waits for completion
  of all outstanding rx operations. Packets in
  RX path will either complete safely or drop.

Signed-off-by: Eryk Kubanski <e.kubanski@partner.samsung.com>
---
 include/net/xdp_sock.h      | 3 ---
 include/net/xsk_buff_pool.h | 2 ++
 net/xdp/xsk.c               | 6 +++---
 net/xdp/xsk_buff_pool.c     | 1 +
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index bfe625b55d55..df3f5f07bc7c 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -71,9 +71,6 @@ struct xdp_sock {
 	 */
 	u32 tx_budget_spent;
 
-	/* Protects generic receive. */
-	spinlock_t rx_lock;
-
 	/* Statistics */
 	u64 rx_dropped;
 	u64 rx_queue_full;
diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index 50779406bc2d..7f0a75d6563d 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -53,6 +53,8 @@ struct xsk_buff_pool {
 	refcount_t users;
 	struct xdp_umem *umem;
 	struct work_struct work;
+	/* Protects generic receive in shared and non-shared umem mode. */
+	spinlock_t rx_lock;
 	struct list_head free_list;
 	struct list_head xskb_list;
 	u32 heads_cnt;
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 89d2bef96469..e2a75f3be237 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -337,13 +337,14 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
 	u32 len = xdp_get_buff_len(xdp);
 	int err;
 
-	spin_lock_bh(&xs->rx_lock);
 	err = xsk_rcv_check(xs, xdp, len);
 	if (!err) {
+		spin_lock_bh(&xs->pool->rx_lock);
 		err = __xsk_rcv(xs, xdp, len);
 		xsk_flush(xs);
+		spin_unlock_bh(&xs->pool->rx_lock);
 	}
-	spin_unlock_bh(&xs->rx_lock);
+
 	return err;
 }
 
@@ -1724,7 +1725,6 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
 	xs = xdp_sk(sk);
 	xs->state = XSK_READY;
 	mutex_init(&xs->mutex);
-	spin_lock_init(&xs->rx_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 1f7975b49657..3a5f16f53178 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -87,6 +87,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
 	pool->addrs = umem->addrs;
 	pool->tx_metadata_len = umem->tx_metadata_len;
 	pool->tx_sw_csum = umem->flags & XDP_UMEM_TX_SW_CSUM;
+	spin_lock_init(&pool->rx_lock);
 	INIT_LIST_HEAD(&pool->free_list);
 	INIT_LIST_HEAD(&pool->xskb_list);
 	INIT_LIST_HEAD(&pool->xsk_tx_list);
-- 
2.34.1

Re: [PATCH] xsk: Fix race condition in AF_XDP generic RX path

[Magnus Karlsson]

On Wed, 9 Apr 2025 at 14:56, e.kubanski <e.kubanski@partner.samsung.com> wrote:
>
> rx_lock moved from xsk_socket to xsk_buff_pool.
> Previous synchronization didn't take care of
> shared umem mode in generic RX path where sockets
> share the same xsk_buff_pool.
>
> RX queue is exclusive to xsk_socket, while FILL
> queue can be shared between multiple sockets.
> This could result in race condition where two
> CPU cores access RX path of two different sockets
> sharing the same umem.

I do not fully understand what you are doing in user space. Could you
please provide a user-space code example that will trigger this
problem?

Please note that if you share an Rx ring or the fill ring between
processes/threads, then you have to take care about mutual exclusion
in user space. If you really want to do this, it is usually a better
idea to use the other shared umem mode in which each process gets its
own rx and fill ring, removing the need for mutual exclusion.

> Now both queues are protected by acquiring spinlock
> in shared xsk_buff_pool.
>
> Lock contention may be minimized in the future by some
> per-thread FQ buffering.
>
> It's safe and necessary to move spin_lock_bh(rx_lock)
> after xsk_rcv_check():
> * xs->pool and spinlock_init is synchronized by
>   xsk_bind() -> xsk_is_bound() memory barriers.
> * xsk_rcv_check() may return true at the moment
>   of xsk_release() or xsk_unbind_dev(),
>   however this will not cause any data races or
>   race conditions. xsk_unbind_dev() removes xdp
>   socket from all maps and waits for completion
>   of all outstanding rx operations. Packets in
>   RX path will either complete safely or drop.
>
> Signed-off-by: Eryk Kubanski <e.kubanski@partner.samsung.com>
> ---
>  include/net/xdp_sock.h      | 3 ---
>  include/net/xsk_buff_pool.h | 2 ++
>  net/xdp/xsk.c               | 6 +++---
>  net/xdp/xsk_buff_pool.c     | 1 +
>  4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index bfe625b55d55..df3f5f07bc7c 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -71,9 +71,6 @@ struct xdp_sock {
>          */
>         u32 tx_budget_spent;
>
> -       /* Protects generic receive. */
> -       spinlock_t rx_lock;
> -
>         /* Statistics */
>         u64 rx_dropped;
>         u64 rx_queue_full;
> diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> index 50779406bc2d..7f0a75d6563d 100644
> --- a/include/net/xsk_buff_pool.h
> +++ b/include/net/xsk_buff_pool.h
> @@ -53,6 +53,8 @@ struct xsk_buff_pool {
>         refcount_t users;
>         struct xdp_umem *umem;
>         struct work_struct work;
> +       /* Protects generic receive in shared and non-shared umem mode. */
> +       spinlock_t rx_lock;
>         struct list_head free_list;
>         struct list_head xskb_list;
>         u32 heads_cnt;
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 89d2bef96469..e2a75f3be237 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -337,13 +337,14 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
>         u32 len = xdp_get_buff_len(xdp);
>         int err;
>
> -       spin_lock_bh(&xs->rx_lock);
>         err = xsk_rcv_check(xs, xdp, len);
>         if (!err) {
> +               spin_lock_bh(&xs->pool->rx_lock);
>                 err = __xsk_rcv(xs, xdp, len);
>                 xsk_flush(xs);
> +               spin_unlock_bh(&xs->pool->rx_lock);
>         }
> -       spin_unlock_bh(&xs->rx_lock);
> +
>         return err;
>  }
>
> @@ -1724,7 +1725,6 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
>         xs = xdp_sk(sk);
>         xs->state = XSK_READY;
>         mutex_init(&xs->mutex);
> -       spin_lock_init(&xs->rx_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 1f7975b49657..3a5f16f53178 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -87,6 +87,7 @@ struct xsk_buff_pool *xp_create_and_assign_umem(struct xdp_sock *xs,
>         pool->addrs = umem->addrs;
>         pool->tx_metadata_len = umem->tx_metadata_len;
>         pool->tx_sw_csum = umem->flags & XDP_UMEM_TX_SW_CSUM;
> +       spin_lock_init(&pool->rx_lock);
>         INIT_LIST_HEAD(&pool->free_list);
>         INIT_LIST_HEAD(&pool->xskb_list);
>         INIT_LIST_HEAD(&pool->xsk_tx_list);
> --
> 2.34.1
>
>

Re: Re: [PATCH] xsk: Fix race condition in AF_XDP generic RX path

[e.kubanski]

> I do not fully understand what you are doing in user space. Could you
> please provide a user-space code example that will trigger this
> problem?

We want to scale single hardware queue AF_XDP setup to
receive packets on multiple threads through RPS mechanisms.
The problem arises when RPS is enabled in the kernel.
In this situation single hardware queue flow can scale across
multiple CPU cores. Then we perform XDP/eBPF load-balancing
to multiple sockets, by using CPU_ID of issued XDP call.

Every socket is binded to queue number 0, device has single queue.

User-space socket setup looks more-or-less like that (with libxdp):
```
xsk_ring_prod fq{};
xsk_ring_cons cq{};

xsk_umem_config umem_cfg{ ... };
xsk_umem* umem;
auto result = xsk_umem__create(&umem, umem_memory, pool_size_bytes, &fq, &cq, &umem_cfg);

...

xsk_socket_config xsk_cfg{
    ...
    .xdp_flags = XDP_FLAGS_SKB_MODE,
    ...
};

xsk_socket* sock1{nullptr};
xsk_ring_cons rq1{};
xsk_ring_prod tq1{};
auto result = xsk_socket__create_shared(
    &sock1,
    device_name,
    0,
    &rq1,
    &tq1,
    &fq,
    &cq,
    &cfg
);

xsk_socket* sock2{nullptr};
xsk_ring_cons rq2{};
xsk_ring_prod tq2{};
auto result = xsk_socket__create_shared(
    &sock2,
    device_name,
    0,
    &rq2,
    &tq2,
    &fq,
    &cq,
    &cfg
);

...
```

We're working on cloud native deploymetns, where
it's not possible to scale RX through RSS mechanism only.

That's why we wanted to use RPS to scale not only
user-space processing but also XDP processing.

This patch effectively allows us to use RPS to scale XDP
in Generic mode.

The same goes for RPS disabled, where we use MACVLAN

child device attached to parent device with multiple queues.
In this situation MACVLAN allows for multi-core kernel-side
processing, but xsk_buff_pool isn't protected.

We can't do any passthrough in this situation, we must rely
on MACVLAN with single RX/TX queue pair.

Of course this is not a problem in situation where every device
packet is processed on single core.

> Please note that if you share an Rx ring or the fill ring between
> processes/threads, then you have to take care about mutual exclusion
> in user space.

Of course, RX/TX/FILL/COMP are SPSC queues, we included mutual
exclusion for FILL/COMP because RX/TX are accessed by single thread.
Im doing single process deployment with multiple threads, where every
thread has it's own AF_XDP socket and pool is shared across threads.

> If you really want to do this, it is usually a better
> idea to use the other shared umem mode in which each process gets its
> own rx and fill ring, removing the need for mutual exclusion.

If I understand AF_XDP architecture correctly it's not possible for single
queue deployment, or maybe Im missing something? We need to maintain
single FILL/COMP pair per device queue.

Re: Re: [PATCH] xsk: Fix race condition in AF_XDP generic RX path

[Magnus Karlsson]

On Wed, 9 Apr 2025 at 16:21, e.kubanski <e.kubanski@partner.samsung.com> wrote:
>
> > I do not fully understand what you are doing in user space. Could you
> > please provide a user-space code example that will trigger this
> > problem?
>
> We want to scale single hardware queue AF_XDP setup to
> receive packets on multiple threads through RPS mechanisms.
> The problem arises when RPS is enabled in the kernel.
> In this situation single hardware queue flow can scale across
> multiple CPU cores. Then we perform XDP/eBPF load-balancing
> to multiple sockets, by using CPU_ID of issued XDP call.
>
> Every socket is binded to queue number 0, device has single queue.
>
> User-space socket setup looks more-or-less like that (with libxdp):
> ```
> xsk_ring_prod fq{};
> xsk_ring_cons cq{};
>
> xsk_umem_config umem_cfg{ ... };
> xsk_umem* umem;
> auto result = xsk_umem__create(&umem, umem_memory, pool_size_bytes, &fq, &cq, &umem_cfg);
>
> ...
>
> xsk_socket_config xsk_cfg{
>     ...
>     .xdp_flags = XDP_FLAGS_SKB_MODE,
>     ...
> };
>
> xsk_socket* sock1{nullptr};
> xsk_ring_cons rq1{};
> xsk_ring_prod tq1{};
> auto result = xsk_socket__create_shared(
>     &sock1,
>     device_name,
>     0,
>     &rq1,
>     &tq1,
>     &fq,
>     &cq,
>     &cfg
> );
>
> xsk_socket* sock2{nullptr};
> xsk_ring_cons rq2{};
> xsk_ring_prod tq2{};
> auto result = xsk_socket__create_shared(
>     &sock2,
>     device_name,
>     0,
>     &rq2,
>     &tq2,
>     &fq,
>     &cq,
>     &cfg
> );
>
> ...
> ```
>
> We're working on cloud native deploymetns, where
> it's not possible to scale RX through RSS mechanism only.
>
> That's why we wanted to use RPS to scale not only
> user-space processing but also XDP processing.
>
> This patch effectively allows us to use RPS to scale XDP
> in Generic mode.
>
> The same goes for RPS disabled, where we use MACVLAN
>
> child device attached to parent device with multiple queues.
> In this situation MACVLAN allows for multi-core kernel-side
> processing, but xsk_buff_pool isn't protected.
>
> We can't do any passthrough in this situation, we must rely
> on MACVLAN with single RX/TX queue pair.
>
> Of course this is not a problem in situation where every device
> packet is processed on single core.

Thanks, this really helped. You are correct that there is a race and
that the previous fix (6 years ago) did not take into account this
shared umem case. I am fine with your fix, just a few things you need
to add to the patch and resubmit a v2.

* Write [PATCH bpf] in your subject and base your code on the bpf
tree, if you did not do that already.

* You need to add a Fixes tag.

And yes, let us worry about performance at some later stage, if it
needs addressing at all. The important thing for this patch is to make
the code solid.

Thank you for spotting this. Highly appreciated.

> > Please note that if you share an Rx ring or the fill ring between
> > processes/threads, then you have to take care about mutual exclusion
> > in user space.
>
> Of course, RX/TX/FILL/COMP are SPSC queues, we included mutual
> exclusion for FILL/COMP because RX/TX are accessed by single thread.
> Im doing single process deployment with multiple threads, where every
> thread has it's own AF_XDP socket and pool is shared across threads.

Good. Just wanted to make sure.

> > If you really want to do this, it is usually a better
> > idea to use the other shared umem mode in which each process gets its
> > own rx and fill ring, removing the need for mutual exclusion.
>
> If I understand AF_XDP architecture correctly it's not possible for single
> queue deployment, or maybe Im missing something? We need to maintain
> single FILL/COMP pair per device queue.

This is correct. You cannot use that mode in your setup.

>