[PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[Magnus Karlsson]

Revert "xsk: support redirect to any socket bound to the same umem"

This patch introduced a potential kernel crash when multiple napi
instances redirect to the same AF_XDP socket. By removing the
queue_index check, it is possible for multiple napi instances to
access the Rx ring at the same time, which will result in a corrupted
ring state which can lead to a crash when flushing the rings in
__xsk_flush(). This can happen when the linked list of sockets to
flush gets corrupted by concurrent accesses. A quick and small fix is
unfortunately not possible, so let us revert this for now.

[  306.997548] BUG: kernel NULL pointer dereference, address: 0000000000000008
[  307.088372] #PF: supervisor read access in kernel mode
[  307.149079] #PF: error_code(0x0000) - not-present page
[  307.209774] PGD 10f131067 P4D 10f131067 PUD 102642067 PMD 0
[  307.276608] Oops: 0000 [#1] SMP
[  307.313712] CPU: 3 PID: 1919 Comm: sp1 Tainted: P           OE     5.15.117-1-ULP-NG #1
[  307.408219] Hardware name: Radware Radware/Default string, BIOS 5.25 (785A.015) 05/11/2023
[  307.505779] RIP: 0010:xsk_flush+0xb/0x40
[  307.552099] Code: a0 03 00 00 01 b8 e4 ff ff ff eb dc 49 83 85 a0 03 00 00 01 b8 e4 ff ff ff eb cd 0f 1f 40 00 48 8b 87 40 03 00 00 55 48 89 e5 <8b> 50 08 48 8b 40 10 89 10 48 8b 87 68 03 00 00 48 8b 80 80 00 00
[  307.773694] RSP: 0000:ffffb7ae01037c80 EFLAGS: 00010287
[  307.835401] RAX: 0000000000000000 RBX: ffffa0a88f8ab768 RCX: ffffa0a88f8abac0
[  307.919670] RDX: ffffa0a88f8abac0 RSI: 0000000000000004 RDI: ffffa0a88f8ab768
[  308.003922] RBP: ffffb7ae01037c80 R08: ffffa0a10b3e0000 R09: 000000000000769f
[  308.088172] R10: ffffa0a1035ca000 R11: 000000000d7f9180 R12: ffffa0a88f8ab768
[  308.172405] R13: ffffa0a88f8ebac0 R14: ffffa0a2ef135300 R15: 0000000000000155
[  308.256635] FS:  00007ffff7e97a80(0000) GS:ffffa0a88f8c0000(0000) knlGS:0000000000000000
[  308.352186] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  308.420043] CR2: 0000000000000008 CR3: 000000010cf6e000 CR4: 0000000000750ee0
[  308.504309] PKRU: 55555554
[  308.536296] Call Trace:
[  308.565209]  <TASK>
[  308.590026]  ? show_regs+0x56/0x60
[  308.630218]  ? __die_body+0x1a/0x60
[  308.671433]  ? __die+0x25/0x30
[  308.707529]  ? page_fault_oops+0xc0/0x440
[  308.754897]  ? do_sys_poll+0x47c/0x5e0
[  308.799188]  ? do_user_addr_fault+0x319/0x6e0
[  308.850659]  ? exc_page_fault+0x6c/0x130
[  308.896992]  ? asm_exc_page_fault+0x27/0x30
[  308.946398]  ? xsk_flush+0xb/0x40
[  308.985546]  __xsk_map_flush+0x3a/0x80
[  309.029824]  xdp_do_flush+0x13/0x20
[  309.071043]  i40e_finalize_xdp_rx+0x44/0x50 [i40e]
[  309.127653]  i40e_clean_rx_irq_zc+0x132/0x500 [i40e]
[  309.202736]  i40e_napi_poll+0x119/0x1270 [i40e]
[  309.256285]  ? xsk_sendmsg+0xf4/0x100
[  309.315969]  ? sock_sendmsg+0x2e/0x40
[  309.359244]  __napi_poll+0x23/0x160
[  309.400482]  net_rx_action+0x232/0x290
[  309.444778]  __do_softirq+0xd0/0x270
[  309.487012]  irq_exit_rcu+0x74/0xa0
[  309.528241]  common_interrupt+0x83/0xa0
[  309.573577]  asm_common_interrupt+0x27/0x40

Thanks: Magnus

Magnus Karlsson (2):
  Revert "xsk: support redirect to any socket bound to the same umem"
  Revert "xsk: document ability to redirect to any socket bound to the
    same umem"

 Documentation/networking/af_xdp.rst | 33 ++++++++++++-----------------
 net/xdp/xsk.c                       |  5 +----
 2 files changed, 15 insertions(+), 23 deletions(-)


base-commit: 2317dc2c22cc353b699c7d1db47b2fe91f54055c
--
2.45.1

[PATCH bpf 1/2] Revert "xsk: support redirect to any socket bound to the same umem"

[Magnus Karlsson]

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

This reverts commit 2863d665ea41282379f108e4da6c8a2366ba66db.

This patch introduced a potential kernel crash when multiple napi
instances redirect to the same AF_XDP socket. By removing the
queue_index check, it is possible for multiple napi instances to
access the Rx ring at the same time, which will result in a corrupted
ring state which can lead to a crash when flushing the rings in
__xsk_flush(). This can happen when the linked list of sockets to
flush gets corrupted by concurrent accesses. A quick and small fix is
not possible, so let us revert this for now.

Reported-by: Yuval El-Hanany <YuvalE@radware.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Link: https://lore.kernel.org/xdp-newbies/8100DBDC-0B7C-49DB-9995-6027F6E63147@radware.com/
---
 net/xdp/xsk.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 727aa20be4bd..7d1c0986f9bb 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -313,13 +313,10 @@ static bool xsk_is_bound(struct xdp_sock *xs)
 
 static int xsk_rcv_check(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 {
-	struct net_device *dev = xdp->rxq->dev;
-	u32 qid = xdp->rxq->queue_index;
-
 	if (!xsk_is_bound(xs))
 		return -ENXIO;
 
-	if (!dev->_rx[qid].pool || xs->umem != dev->_rx[qid].pool->umem)
+	if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
 		return -EINVAL;
 
 	if (len > xsk_pool_get_rx_frame_size(xs->pool) && !xs->sg) {
-- 
2.45.1

[PATCH bpf 2/2] Revert "xsk: document ability to redirect to any socket bound to the same umem"

[Magnus Karlsson]

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

This reverts commit 968595a93669b6b4f6d1fcf80cf2d97956b6868f.

Reported-by: Yuval El-Hanany <YuvalE@radware.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Link: https://lore.kernel.org/xdp-newbies/8100DBDC-0B7C-49DB-9995-6027F6E63147@radware.com/
---
 Documentation/networking/af_xdp.rst | 33 ++++++++++++-----------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/Documentation/networking/af_xdp.rst b/Documentation/networking/af_xdp.rst
index 72da7057e4cf..dceeb0d763aa 100644
--- a/Documentation/networking/af_xdp.rst
+++ b/Documentation/networking/af_xdp.rst
@@ -329,24 +329,23 @@ XDP_SHARED_UMEM option and provide the initial socket's fd in the
 sxdp_shared_umem_fd field as you registered the UMEM on that
 socket. These two sockets will now share one and the same UMEM.
 
-In this case, it is possible to use the NIC's packet steering
-capabilities to steer the packets to the right queue. This is not
-possible in the previous example as there is only one queue shared
-among sockets, so the NIC cannot do this steering as it can only steer
-between queues.
-
-In libxdp (or libbpf prior to version 1.0), you need to use the
-xsk_socket__create_shared() API as it takes a reference to a FILL ring
-and a COMPLETION ring that will be created for you and bound to the
-shared UMEM. You can use this function for all the sockets you create,
-or you can use it for the second and following ones and use
-xsk_socket__create() for the first one. Both methods yield the same
-result.
+There is no need to supply an XDP program like the one in the previous
+case where sockets were bound to the same queue id and
+device. Instead, use the NIC's packet steering capabilities to steer
+the packets to the right queue. In the previous example, there is only
+one queue shared among sockets, so the NIC cannot do this steering. It
+can only steer between queues.
+
+In libbpf, you need to use the xsk_socket__create_shared() API as it
+takes a reference to a FILL ring and a COMPLETION ring that will be
+created for you and bound to the shared UMEM. You can use this
+function for all the sockets you create, or you can use it for the
+second and following ones and use xsk_socket__create() for the first
+one. Both methods yield the same result.
 
 Note that a UMEM can be shared between sockets on the same queue id
 and device, as well as between queues on the same device and between
-devices at the same time. It is also possible to redirect to any
-socket as long as it is bound to the same umem with XDP_SHARED_UMEM.
+devices at the same time.
 
 XDP_USE_NEED_WAKEUP bind flag
 -----------------------------
@@ -823,10 +822,6 @@ A: The short answer is no, that is not supported at the moment. The
    switch, or other distribution mechanism, in your NIC to direct
    traffic to the correct queue id and socket.
 
-   Note that if you are using the XDP_SHARED_UMEM option, it is
-   possible to switch traffic between any socket bound to the same
-   umem.
-
 Q: My packets are sometimes corrupted. What is wrong?
 
 A: Care has to be taken not to feed the same buffer in the UMEM into
-- 
2.45.1

Re: [PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[Willem de Bruijn]

Magnus Karlsson wrote:
> Revert "xsk: support redirect to any socket bound to the same umem"
> 
> This patch introduced a potential kernel crash when multiple napi
> instances redirect to the same AF_XDP socket. By removing the
> queue_index check, it is possible for multiple napi instances to
> access the Rx ring at the same time, which will result in a corrupted
> ring state which can lead to a crash when flushing the rings in
> __xsk_flush(). This can happen when the linked list of sockets to
> flush gets corrupted by concurrent accesses. A quick and small fix is
> unfortunately not possible, so let us revert this for now.

This is a very useful feature, to be able to use AF_XDP sockets with
a standard RSS nic configuration.

Not all AF_XDP use cases require the absolute highest packet rate.

Can this be addressed with an optional spinlock on the RxQ, only for
this case?

If there is no simple enough fix in the short term, do you plan to
reintroduce this in another form later?

Re: [PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[Magnus Karlsson]

On Wed, 5 Jun 2024 at 01:03, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Magnus Karlsson wrote:
> > Revert "xsk: support redirect to any socket bound to the same umem"
> >
> > This patch introduced a potential kernel crash when multiple napi
> > instances redirect to the same AF_XDP socket. By removing the
> > queue_index check, it is possible for multiple napi instances to
> > access the Rx ring at the same time, which will result in a corrupted
> > ring state which can lead to a crash when flushing the rings in
> > __xsk_flush(). This can happen when the linked list of sockets to
> > flush gets corrupted by concurrent accesses. A quick and small fix is
> > unfortunately not possible, so let us revert this for now.
>
> This is a very useful feature, to be able to use AF_XDP sockets with
> a standard RSS nic configuration.

I completely agree.

> Not all AF_XDP use cases require the absolute highest packet rate.
>
> Can this be addressed with an optional spinlock on the RxQ, only for
> this case?

Yes, or with a MPSC ring implementation.

> If there is no simple enough fix in the short term, do you plan to
> reintroduce this in another form later?

Yuval and I are looking into a solution based around an optional
spinlock since it is easier to pull off than an MPSC ring. The
discussion is on-going on the xdp-newbies list [0], but as soon as we
have a first patch, we will post it here for review and debate.

[0] https://lore.kernel.org/xdp-newbies/8100DBDC-0B7C-49DB-9995-6027F6E63147@radware.com/

Re: [PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue,  4 Jun 2024 14:29:24 +0200 you wrote:
> Revert "xsk: support redirect to any socket bound to the same umem"
> 
> This patch introduced a potential kernel crash when multiple napi
> instances redirect to the same AF_XDP socket. By removing the
> queue_index check, it is possible for multiple napi instances to
> access the Rx ring at the same time, which will result in a corrupted
> ring state which can lead to a crash when flushing the rings in
> __xsk_flush(). This can happen when the linked list of sockets to
> flush gets corrupted by concurrent accesses. A quick and small fix is
> unfortunately not possible, so let us revert this for now.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] Revert "xsk: support redirect to any socket bound to the same umem"
    https://git.kernel.org/bpf/bpf/c/7fcf26b315bb
  - [bpf,2/2] Revert "xsk: document ability to redirect to any socket bound to the same umem"
    https://git.kernel.org/bpf/bpf/c/03e38d315f3c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf 0/2] Revert "xsk: support redirect to any socket bound to the same umem"

[Willem de Bruijn]

Magnus Karlsson wrote:
> On Wed, 5 Jun 2024 at 01:03, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Magnus Karlsson wrote:
> > > Revert "xsk: support redirect to any socket bound to the same umem"
> > >
> > > This patch introduced a potential kernel crash when multiple napi
> > > instances redirect to the same AF_XDP socket. By removing the
> > > queue_index check, it is possible for multiple napi instances to
> > > access the Rx ring at the same time, which will result in a corrupted
> > > ring state which can lead to a crash when flushing the rings in
> > > __xsk_flush(). This can happen when the linked list of sockets to
> > > flush gets corrupted by concurrent accesses. A quick and small fix is
> > > unfortunately not possible, so let us revert this for now.
> >
> > This is a very useful feature, to be able to use AF_XDP sockets with
> > a standard RSS nic configuration.
> 
> I completely agree.
> 
> > Not all AF_XDP use cases require the absolute highest packet rate.
> >
> > Can this be addressed with an optional spinlock on the RxQ, only for
> > this case?
> 
> Yes, or with a MPSC ring implementation.
> 
> > If there is no simple enough fix in the short term, do you plan to
> > reintroduce this in another form later?
> 
> Yuval and I are looking into a solution based around an optional
> spinlock since it is easier to pull off than an MPSC ring. The
> discussion is on-going on the xdp-newbies list [0], but as soon as we
> have a first patch, we will post it here for review and debate.
> 
> [0] https://lore.kernel.org/xdp-newbies/8100DBDC-0B7C-49DB-9995-6027F6E63147@radware.com/

Glad to hear that it's intended to be supported, and even being worked
on, thanks! I'll follow the conversation there.