Re: stmmac and XDP/ZC issue

Serge Semin
2024-02-22T10:06:40Z

On Thu, Feb 22, 2024 at 09:35:02AM +0100, Kurt Kanzenbach wrote:
> On Wed Feb 21 2024, Serge Semin wrote:
> > On Wed, Feb 21, 2024 at 04:59:10PM +0100, Maciej Fijalkowski wrote:
> >> On Wed, Feb 21, 2024 at 10:21:04AM +0100, Kurt Kanzenbach wrote:
> >> > On Wed Feb 21 2024, Kurt Kanzenbach wrote:
> >> > > On Tue Feb 20 2024, Stanislav Fomichev wrote:
> >> > >> On Tue, Feb 20, 2024 at 6:43 AM Maciej Fijalkowski
> >> > >> <maciej.fijalkowski@intel.com> wrote:
> >> > >>>
> >> > >>> On Tue, Feb 20, 2024 at 04:18:54PM +0300, Serge Semin wrote:
> >> > >>> > Hi Kurt
> >> > >>> >
> >> > >>> > On Tue, Feb 20, 2024 at 12:02:25PM +0100, Kurt Kanzenbach wrote:
> >> > >>> > > Hello netdev community,
> >> > >>> > >
> >> > >>> > > after updating to v6.8 kernel I've encountered an issue in the stmmac
> >> > >>> > > driver.
> >> > >>> > >
> >> > >>> > > I have an application which makes use of XDP zero-copy sockets. It works
> >> > >>> > > on v6.7. On v6.8 it results in the stack trace shown below. The program
> >> > >>> > > counter points to:
> >> > >>> > >
> >> > >>> > > - ./include/net/xdp_sock.h:192 and
> >> > >>> > > - ./drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:2681
> >> > >>> > >
> >> > >>> > > It seems to be caused by the XDP meta data patches. This one in
> >> > >>> > > particular 1347b419318d ("net: stmmac: Add Tx HWTS support to XDP ZC").
> >> > >>> > >
> >> > >>> > > To reproduce:
> >> > >>> > >
> >> > >>> > > - Hardware: imx93
> >> > >>> > > - Run ptp4l/phc2sys
> >> > >>> > > - Configure Qbv, Rx steering, NAPI threading
> >> > >>> > > - Run my application using XDP/ZC on queue 1
> >> > >>> > >
> >> > >>> > > Any idea what might be the issue here?
> >> > >>> > >
> >> > >>> > > Thanks,
> >> > >>> > > Kurt
> >> > >>> > >
> >> > >>> > > Stack trace:
> >> > >>> > >
> >> > >>> > > |[ 169.248150] imx-dwmac 428a0000.ethernet eth1: configured EST
> >> > >>> > > |[ 191.820913] imx-dwmac 428a0000.ethernet eth1: EST: SWOL has been switched
> >> > >>> > > |[ 226.039166] imx-dwmac 428a0000.ethernet eth1: entered promiscuous mode
> >> > >>> > > |[ 226.203262] imx-dwmac 428a0000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-0
> >> > >>> > > |[ 226.203753] imx-dwmac 428a0000.ethernet eth1: Register MEM_TYPE_PAGE_POOL RxQ-1
> >> > >>> > > |[ 226.303337] imx-dwmac 428a0000.ethernet eth1: Register MEM_TYPE_XSK_BUFF_POOL RxQ-1
> >> > >>> > > |[ 255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> >> > >>> > > |[ 255.822602] Mem abort info:
> >> > >>> > > |[ 255.822604] ESR = 0x0000000096000044
> >> > >>> > > |[ 255.822608] EC = 0x25: DABT (current EL), IL = 32 bits
> >> > >>> > > |[ 255.822613] SET = 0, FnV = 0
> >> > >>> > > |[ 255.822616] EA = 0, S1PTW = 0
> >> > >>> > > |[ 255.822618] FSC = 0x04: level 0 translation fault
> >> > >>> > > |[ 255.822622] Data abort info:
> >> > >>> > > |[ 255.822624] ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
> >> > >>> > > |[ 255.822627] CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> >> > >>> > > |[ 255.822630] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> >> > >>> > > |[ 255.822634] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000085fe1000
> >> > >>> > > |[ 255.822638] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> >> > >>> > > |[ 255.822650] Internal error: Oops: 0000000096000044 [#1] PREEMPT_RT SMP
> >> > >>> > > |[ 255.822655] Modules linked in:
> >> > >>> > > |[ 255.822660] CPU: 0 PID: 751 Comm: napi/eth1-261 Not tainted 6.8.0-rc4-rt4-00100-g9c63d995ca19 #8
> >> > >>> > > |[ 255.822666] Hardware name: NXP i.MX93 11X11 EVK board (DT)
> >> > >>> > > |[ 255.822669] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >> > >>> > > |[ 255.822674] pc : stmmac_tx_clean.constprop.0+0x848/0xc38
> >> > >>> > > |[ 255.822690] lr : stmmac_tx_clean.constprop.0+0x844/0xc38
> >> > >>> > > |[ 255.822696] sp : ffff800085ec3bc0
> >> > >>> > > |[ 255.822698] x29: ffff800085ec3bc0 x28: ffff000005b609e0 x27: 0000000000000001
> >> > >>> > > |[ 255.822706] x26: 0000000000000000 x25: ffff000005b60ae0 x24: 0000000000000001
> >> > >>> > > |[ 255.822712] x23: 0000000000000001 x22: ffff000005b649e0 x21: 0000000000000000
> >> > >>> > > |[ 255.822719] x20: 0000000000000020 x19: ffff800085291030 x18: 0000000000000000
> >> > >>> > > |[ 255.822725] x17: ffff7ffffc51c000 x16: ffff800080000000 x15: 0000000000000008
> >> > >>> > > |[ 255.822732] x14: ffff80008369b880 x13: 0000000000000000 x12: 0000000000008507
> >> > >>> > > |[ 255.822738] x11: 0000000000000040 x10: 0000000000000a70 x9 : ffff800080e32f84
> >> > >>> > > |[ 255.822745] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0000000000003ff0
> >> > >>> > > |[ 255.822751] x5 : 0000000000003c40 x4 : ffff000005b60000 x3 : 0000000000000000
> >> > >>> > > |[ 255.822757] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
> >> > >>> > > |[ 255.822764] Call trace:
> >> > >>> > > |[ 255.822766] stmmac_tx_clean.constprop.0+0x848/0xc38
> >> > >>>
> >> > >>> Shouldn't xsk_tx_metadata_complete() be called only when corresponding
> >> > >>> buf_type is STMMAC_TXBUF_T_XSK_TX?
> >> > >>
> >> > >> +1. I'm assuming Serge isn't enabling it explicitly, so none of the
> >> > >> metadata stuff should trigger in this case.
> >> > >
> >> > > The only other user of xsk_tx_metadata_complete() in mlx5 guards it with
> >> > > xp_tx_metadata_enabled(). Seems like that's missing in stmmac?
> >> >
> >> > Well, the following patch seems to help:
> >> >
> >> > commit e85ab4b97b4d6e50036435ac9851b876c221f580
> >> > Author: Kurt Kanzenbach <kurt@linutronix.de>
> >> > Date: Wed Feb 21 08:18:15 2024 +0100
> >> >
> >> > net: stmmac: Complete meta data only when enabled
> >> >
> >> > Currently using XDP sockets on stmmac results in a kernel crash:
> >> >
> >> > |[ 255.822584] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> >> > |[...]
> >> > |[ 255.822764] Call trace:
> >> > |[ 255.822766] stmmac_tx_clean.constprop.0+0x848/0xc38
> >> >
> >> > The program counter indicates xsk_tx_metadata_complete(). However, this
> >> > function shouldn't be called unless metadata is actually enabled.
> >> >
> >> > Tested on imx93.
> >> >
> >> > Fixes: 1347b419318d ("net: stmmac: Add Tx HWTS support to XDP ZC")
> >> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> >> >
> >> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > index 9df27f03a8cb..77c62b26342d 100644
> >> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > @@ -2678,9 +2678,10 @@ static int stmmac_tx_clean(struct stmmac_priv *priv, int budget, u32 queue,
> >> > .desc = p,
> >> > };
> >> >
> >> > - xsk_tx_metadata_complete(&tx_q->tx_skbuff_dma[entry].xsk_meta,
> >> > - &stmmac_xsk_tx_metadata_ops,
> >> > - &tx_compl);
> >> > + if (xp_tx_metadata_enabled(tx_q->xsk_pool))
> >>
> >
> >> every other usage of tx metadata functions should be wrapped with
> >> xp_tx_metadata_enabled() - can you address other places and send a proper
> >> patch?
> >
> > AFAICS this is the only place. But the change above still isn't enough
> > to fix the problem. In my case XDP zero-copy isn't activated. So
> > xsk_pool isn't allocated and the NULL/~NULL dereference is still
> > persistent due to xp_tx_metadata_enabled() dereferencing the
> > NULL-structure fields. The attached patched fixes the problem in my
> > case.
>
> Sure about that? In my case without ZC the else path is not executed,
> because skb is set.

Absolutely. Don't know why you haven't got the NULL-dereference bug as
I have. I was able to track the bug up to not having the
stmmac_rx_queue::xsk_pool allocated. Maybe your case is different in a
aspect that the pool had been pre-allocated someway before the ZC was
disabled. Anyway I agree it will be safer to keep the xsk_pool pointer
sanity check as it's done in the rest of the places in the driver.

>
> >
> > Kurt, are you sure that xp_tx_metadata_enabled() is required in your
> > case?
>
> Yes, I'm sure it's required, because I do use ZC without using any
> metadata.

Ok.

>
> > Could you test the attached patch with the xp_tx_metadata_enabled()
> > invocation discarded?
>
> Well, it works. But, the xp_tx_metadata_enabled() is not discarded in
> the ZC case:
>
> |RtcRxThread-790 [001] b...3 202.970243: stmmac_tx_clean.constprop.0: huhu from xp_tx_metadata_enabled
>
> Let's go with your version of the patch. It works without XDP, with XDP
> and XDP/ZC. I'll send it upstream.
>
> Thanks for the help :).

Thanks for posting the patch. Here is the link for the lore archive:
https://lore.kernel.org/netdev/20240222-stmmac_xdp-v1-1-e8d2d2b79ff0@linutronix.de

-Serge(y)

>
> Thanks,
> Kurt