Re: [Intel-wired-lan] [RFC PATCHv2 1/1] idpf: Fix header clobber in IDPF with SWIOTLB enabled

Steve Rutherford via Intel-wired-lan
2026-03-04T22:02:30Z

I believe syncing twice isn't inherently wrong - it's more that you
can't synthesize the header via the workaround and then sync, since it
will pull the uninitialized header buffer from the SWIOTLB. Outside of
SWIOTLB, dma syncs are more or less no-ops, while (with SWIOTLB) they
are copies from/to the bounce buffers.

On Wed, Mar 4, 2026 at 7:13 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Steve Rutherford <srutherford@google.com>
> Date: Tue, 3 Mar 2026 11:44:19 -0800
>
> > On Tue, Mar 3, 2026 at 7:34 AM Alexander Lobakin
> > <aleksander.lobakin@intel.com> wrote:
> >>
> >> From: Steve Rutherford <srutherford@google.com>
> >> Date: Fri, 27 Feb 2026 20:34:57 +0000
> >>
> >>> When SWIOTLB and header split are enabled, IDPF sees empty packets in the
> >>> rx queue.
> >>>
> >>> This is caused by libeth_rx_sync_for_cpu clobbering the synthesized header
> >>> in the workaround (i.e. overflow) path. After the header is synthesized by
> >>> idpf_rx_hsplit_wa, the sync call pulls from the empty SWIOTLB buffer,
> >>> effectively zeroing out the buffer.
> >>>
> >>> This skips the extra sync in the workaround path in most cases. The one
> >>> exception is that it calls sync to trigger a recycle the header buffer when
> >>> it fails to find a header in the payload.
> >>>
> >>> Fixes: 90912f9f4f2d1 ("idpf: convert header split mode to libeth + napi_build_skb()")
> >>> Signed-off-by: Steve Rutherford <srutherford@google.com>
> >>> ---
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c | 9 +++++++--
> >>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> >>> index 3ddf7b1e85ef..946203a6bd86 100644
> >>> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> >>> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> >>> @@ -3007,9 +3007,14 @@ static int idpf_rx_splitq_clean(struct idpf_rx_queue *rxq, int budget)
> >>> u64_stats_update_begin(&rxq->stats_sync);
> >>> u64_stats_inc(&rxq->q_stats.hsplit_buf_ovf);
> >>> u64_stats_update_end(&rxq->stats_sync);
> >>> - }
> >>>
> >>> - if (libeth_rx_sync_for_cpu(hdr, hdr_len)) {
> >>> + /* Recycle the hdr buffer if unused.*/
> >>> + if (!hdr_len)
> >>> + libeth_rx_sync_for_cpu(hdr, 0);
> >>> + } else if (!libeth_rx_sync_for_cpu(hdr, hdr_len))
> >>> + hdr_len = 0;
> >>> +
> >>> + if (hdr_len) {
> >>
> >> This is for a very old tree I believe? We now have
> >> libeth_xdp_process_buff() there for quite some time already.
> >
> > It is, yeah. I thought I posted a cover letter with more of a description, but,
> > frankly, I may have messed up the process of posting.
> >
> > From the cover letter -
> > Found an issue with the IDPF driver when SWIOTLB is enabled. The issue
> > results in empty headers for packets that hit the split queue workaround
> > path. It's caused by a spurious sync in that path. The header is synced
> > from the SWIOTLB even when the header was shoved into the payload.
> >
> > I cooked up a sample patch, but I'm not an expert in this driver, so I have
> > no idea if it's the right solution. It did allow my QEMU VM to boot with a
> > superficially functional passed-through IDPF NIC and SWIOTLB=force.
> >
> > The patch was written against COS's 6.12, so I assume that it will not
> > apply cleanly elsewhere, but I figured a wrong sample patch was better than
> > a long paragraph describing the same thing. My read of more recent kernels
> > is that this problem is still present, but could be mistaken.
>
> Ooops, sorry, I haven't read the cover letter =\
>
> Did I get it correctly that in case of SWIOTLB, we can't sync the same
> buffer two times? But if the hsplit W/A was applied, then this double
> sync corrupts the data?
>
> I'll prepare a patch for the latest net (with you as Co-developed-by or
> any other tag you prefer) once I find a way how to play this nicely with
> libeth_xdp_process_buff(). It performs an unconditional sync and bails
> out if it returned false.
>
> >
> > Thanks,
> > Steve
>
> Thanks,
> Olek