On Thursday, March 7, 2024 9:39 PM, Kurt Kanzenbach kurt@linutronix.de 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:
4beee3a037e4@linutronix.de/
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