On Thursday, March 7, 2024 9:39 PM, Kurt Kanzenbach <kurt@xxxxxxxxxxxxx> wrote: >Hi Maciej, > >On Wed Mar 06 2024, Maciej Fijalkowski wrote: >> On Sun, Mar 03, 2024 at 04:32:25PM +0800, Song Yoong Siang wrote: >>> - tstamp->skb = NULL; >>> + /* Copy the tx hardware timestamp into xdp metadata or skb */ >>> + if (tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK) >> >> I believe this should also be protected with xp_tx_metadata_enabled() >> check. We recently had following bugfix, PTAL: >> >> https://lore.kernel.org/bpf/20240222-stmmac_xdp-v2-1- >4beee3a037e4@xxxxxxxxxxxxx/ >> >> I'll take a deeper look at patch tomorrow, might be the case that you've >> addressed that or you were aware of this issue but anyways wanted to bring >> it up. Just check that you don't break standard XDP/AF_XDP traffic :) > >This one doesn't crash standard AF_XDP traffic. Don't know why, but it >seems to work. > >Thanks, >Kurt Thanks Maciej and Kurt for the comments. In stmmac, xsk_tx_metadata_complete() is called by generic tx complete irq, thus causing the NULL pointer issue. In igc, xsk_tx_metadata_complete() is called by ptp irq, and we use tstamp->buffer_type == IGC_TX_BUFFER_TYPE_XSK as a flag to check whether XDP Tx metadata is used. Thus other data path won't call into xsk_tx_metadata_complete(). Even it work, but I think I should still add xp_tx_metadata_enabled() checking for safety measure. Any thoughts? In case Maciej have more comments, I will wait few days before I add the checking in v4. Btw, thank for fixing the issue on stmmac. Thanks & Regards Siang
