On Wed, Jan 03, 2024 at 12:48:10PM -0800, John Fastabend wrote:
> Maciej Fijalkowski wrote:
> > Currently when packet is shrunk via bpf_xdp_adjust_tail(), null ptr
> > dereference happens:
> >
> > [1136314.192256] BUG: kernel NULL pointer dereference, address:
> > 0000000000000034
> > [1136314.203943] #PF: supervisor read access in kernel mode
> > [1136314.213768] #PF: error_code(0x0000) - not-present page
> > [1136314.223550] PGD 0 P4D 0
> > [1136314.230684] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [1136314.239621] CPU: 8 PID: 54203 Comm: xdpsock Not tainted 6.6.0+ #257
> > [1136314.250469] Hardware name: Intel Corporation S2600WFT/S2600WFT,
> > BIOS SE5C620.86B.02.01.0008.031920191559 03/19/2019
> > [1136314.265615] RIP: 0010:__xdp_return+0x6c/0x210
> > [1136314.274653] Code: ad 00 48 8b 47 08 49 89 f8 a8 01 0f 85 9b 01 00 00 0f 1f 44 00 00 f0 41 ff 48 34 75 32 4c 89 c7 e9 79 cd 80 ff 83 fe 03 75 17 <f6> 41 34 01 0f 85 02 01 00 00 48 89 cf e9 22 cc 1e 00 e9 3d d2 86
> > [1136314.302907] RSP: 0018:ffffc900089f8db0 EFLAGS: 00010246
> > [1136314.312967] RAX: ffffc9003168aed0 RBX: ffff8881c3300000 RCX:
> > 0000000000000000
> > [1136314.324953] RDX: 0000000000000000 RSI: 0000000000000003 RDI:
> > ffffc9003168c000
> > [1136314.336929] RBP: 0000000000000ae0 R08: 0000000000000002 R09:
> > 0000000000010000
> > [1136314.348844] R10: ffffc9000e495000 R11: 0000000000000040 R12:
> > 0000000000000001
> > [1136314.360706] R13: 0000000000000524 R14: ffffc9003168aec0 R15:
> > 0000000000000001
> > [1136314.373298] FS: 00007f8df8bbcb80(0000) GS:ffff8897e0e00000(0000)
> > knlGS:0000000000000000
> > [1136314.386105] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [1136314.396532] CR2: 0000000000000034 CR3: 00000001aa912002 CR4:
> > 00000000007706f0
> > [1136314.408377] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> > 0000000000000000
> > [1136314.420173] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> > 0000000000000400
> > [1136314.431890] PKRU: 55555554
> > [1136314.439143] Call Trace:
> > [1136314.446058] <IRQ>
> > [1136314.452465] ? __die+0x20/0x70
> > [1136314.459881] ? page_fault_oops+0x15b/0x440
> > [1136314.468305] ? exc_page_fault+0x6a/0x150
> > [1136314.476491] ? asm_exc_page_fault+0x22/0x30
> > [1136314.484927] ? __xdp_return+0x6c/0x210
> > [1136314.492863] bpf_xdp_adjust_tail+0x155/0x1d0
> > [1136314.501269] bpf_prog_ccc47ae29d3b6570_xdp_sock_prog+0x15/0x60
> > [1136314.511263] ice_clean_rx_irq_zc+0x206/0xc60 [ice]
> > [1136314.520222] ? ice_xmit_zc+0x6e/0x150 [ice]
> > [1136314.528506] ice_napi_poll+0x467/0x670 [ice]
> > [1136314.536858] ? ttwu_do_activate.constprop.0+0x8f/0x1a0
> > [1136314.546010] __napi_poll+0x29/0x1b0
> > [1136314.553462] net_rx_action+0x133/0x270
> > [1136314.561619] __do_softirq+0xbe/0x28e
> > [1136314.569303] do_softirq+0x3f/0x60
> >
> > This comes from __xdp_return() call with xdp_buff argument passed as
> > NULL which is supposed to be consumed by xsk_buff_free() call.
> >
> > To address this properly, in ZC case, a node that represents the frag
> > being removed has to be pulled out of xskb_list. Introduce
>
> hmm it looks like xsk_buff_free() called by __xdp_return would
> pull the frag out of the xskb_list? Or am I wrong?
>
> Then the issue is primarily the NULL handling?
Hey John, as you see later on it is also about adjusting the size within
xdp_buff that comes from xsk pool, in case when offset is not bigger than
frag size.
>
> > appriopriate xsk helpers to do such node operation and use them
> > accordingly within bpf_xdp_adjust_tail().
> >
> > Fixes: 24ea50127ecf ("xsk: support mbuf on ZC RX")
> > Acked-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx> # For the xsk header part
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > ---
> > include/net/xdp_sock_drv.h | 26 +++++++++++++++++++++
> > net/core/filter.c | 48 +++++++++++++++++++++++++++++++-------
> > 2 files changed, 65 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> > index b62bb8525a5f..3d35ac0f838b 100644
> > --- a/include/net/xdp_sock_drv.h
> > +++ b/include/net/xdp_sock_drv.h
> > @@ -159,6 +159,23 @@ static inline struct xdp_buff *xsk_buff_get_frag(struct xdp_buff *first)
> > return ret;
> > }
> >
> > +static inline void xsk_buff_del_tail(struct xdp_buff *tail)
> > +{
> > + struct xdp_buff_xsk *xskb = container_of(tail, struct xdp_buff_xsk, xdp);
> > +
> > + list_del(&xskb->xskb_list_node);
> > +}
> > +
> > +static inline struct xdp_buff *xsk_buff_get_tail(struct xdp_buff *first)
> > +{
> > + struct xdp_buff_xsk *xskb = container_of(first, struct xdp_buff_xsk, xdp);
> > + struct xdp_buff_xsk *frag;
> > +
> > + frag = list_last_entry(&xskb->pool->xskb_list, struct xdp_buff_xsk,
> > + xskb_list_node);
> > + return &frag->xdp;
> > +}
> > +
> > static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size)
> > {
> > xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> > @@ -350,6 +367,15 @@ static inline struct xdp_buff *xsk_buff_get_frag(struct xdp_buff *first)
> > return NULL;
> > }
> >
> > +static inline void xsk_buff_del_tail(struct xdp_buff *tail)
> > +{
> > +}
> > +
> > +static inline struct xdp_buff *xsk_buff_get_tail(struct xdp_buff *first)
> > +{
> > + return NULL;
> > +}
> > +
> > static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size)
> > {
> > }
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 24061f29c9dd..1e20196687fd 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -83,6 +83,7 @@
> > #include <net/netfilter/nf_conntrack_bpf.h>
> > #include <net/netkit.h>
> > #include <linux/un.h>
> > +#include <net/xdp_sock_drv.h>
> >
> > #include "dev.h"
> >
> > @@ -4096,6 +4097,42 @@ static int bpf_xdp_frags_increase_tail(struct xdp_buff *xdp, int offset)
> > return 0;
> > }
> >
> > +static void __shrink_data(struct xdp_buff *xdp, struct xdp_mem_info *mem_info,
> > + skb_frag_t *frag, int shrink)
> > +{
> > + if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
> > + struct xdp_buff *tail = xsk_buff_get_tail(xdp);
> > +
> > + if (tail)
> > + tail->data_end -= shrink;
> > + }
> > + skb_frag_size_sub(frag, shrink);
> > +}
> > +
> > +static bool shrink_data(struct xdp_buff *xdp, skb_frag_t *frag, int shrink)
> > +{
> > + struct xdp_mem_info *mem_info = &xdp->rxq->mem;
> > +
> > + if (skb_frag_size(frag) == shrink) {
> > + struct page *page = skb_frag_page(frag);
> > + struct xdp_buff *zc_frag = NULL;
> > +
> > + if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
> > + zc_frag = xsk_buff_get_tail(xdp);
> > +
> > + if (zc_frag) {
> > + xdp_buff_clear_frags_flag(zc_frag);
> > + xsk_buff_del_tail(zc_frag);
> > + }
> > + }
>
> Should this be fixed in xdp_return instead of here? The xdp_return
> is doing what xsk_buff_del_tail() does. If we also called clear_frags
> there could this be simpler?
xsk_buff_del_tail() only deletes node from xskb_list and xsk_buff_free()
in the will call xp_free() on frag being deleted.
I think I would be rather leaning towards adding xp_free() to
xsk_buff_del_tail() and skipping __xdp_return() call from ZC case
altogether...
>
> if (skb_frag_size(frag) == shrink) {
> struct page *page = skb_frag_page(frag);
>
> __xdp_return(page_address(page), mem_info, false, xsk_buff_get_tail(xdp));
> } else {
> __shrink_data(xdp, mem_info, frag, shrink);
> }
>
> the return will need to have an unlikely(!xdp) to guard the case it
> might be NULL, but also not sure if we would ever expect a NULL
> here if MEM_TYPE_XSK_BUFF_POOL so you might skip that unlikely
> as well?
In that approach you would xp_free() the frag being removed but it would
still be dangling in xskb_list that first xdp_buff carries. Some
adjustments would have to be done within xsk_buff_free().
Regarding the NULLness of zc_frag for MEM_TYPE_XSK_BUFF_POOL that Martin
also brought, you are right, I went too far with this, I misread what
kernel test robot reported :) so on monday I will look into this.
>
> > +
> > + __xdp_return(page_address(page), mem_info, false, zc_frag);
> > + return true;
> > + }
> > + __shrink_data(xdp, mem_info, frag, shrink);
> > + return false;
> > +}
> > +
> > static int bpf_xdp_frags_shrink_tail(struct xdp_buff *xdp, int offset)
> > {
> > struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > @@ -4110,17 +4147,10 @@ static int bpf_xdp_frags_shrink_tail(struct xdp_buff *xdp, int offset)
> >
> > len_free += shrink;
> > offset -= shrink;
> > -
> > - if (skb_frag_size(frag) == shrink) {
> > - struct page *page = skb_frag_page(frag);
> > -
>
> And then I likely would avoid the helper altogether? And code
> example above just lands here?
>
> > - __xdp_return(page_address(page), &xdp->rxq->mem,
> > - false, NULL);
> > + if (shrink_data(xdp, frag, shrink))
> > n_frags_free++;
> > - } else {
> > - skb_frag_size_sub(frag, shrink);
> > + else
> > break;
> > - }
> > }
>
> I think the fix can be more straight-forward if we just populate
> the NULL field with the xdp_buff using the get_tail() helper
> created above.
I'll think about both approaches, the one you're suggesting and the other
that I wrote up above. Thanks a lot for taking a look!
>
> > sinfo->nr_frags -= n_frags_free;
> > sinfo->xdp_frags_size -= len_free;
> > --
> > 2.34.1
> >
> >
>
>
>
