netdev - Re: [PATCH net] i40e: xsk: advance next_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANp2VBUo_8dXjjFLmgyP=Wtz65-F_BQ05Bfrz3xB7cs0iW_CyQ@mail.gmail.com>
Date: Wed, 22 Oct 2025 08:59:46 +0700
From: Alessandro Decina <alessandro.d@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: netdev@...r.kernel.org, Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Subject: Re: [PATCH net] i40e: xsk: advance next_to_clean on status descriptors

On Wed, Oct 22, 2025 at 8:16 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>
> Hi Alessandro,

Hi Jason, thanks for the quick review!

> On Tue, Oct 21, 2025 at 9:59 PM Alessandro Decina
> <alessandro.d@...il.com> wrote:
> >
> > Whenever a status descriptor is received, i40e processes and skips over
> > it, correctly updating next_to_process but forgetting to update
> > next_to_clean. In the next iteration this accidentally causes the
> > creation of an invalid multi-buffer xdp_buff where the first fragment
> > is the status descriptor.
>
> Upon a quick review, if the packet is not a normal packet,
> next_to_clean should be advanced by one anyway, right? If so, we can
> only use something like "next_to_clean++". According to what you gave
> us as above, only if that condition is satisfied, the next_to_clean
> will be synced to next_to_process. In other cases, the next_to_clean
> will not be updated. But the packet read by using next_to_clean is one
> status descriptor, should we skip it one way or another?
>
> One more question from my side is that since the first packet should
> not be a status packet, after your patch gets applied, in the while
> loop, 'first' still points to the original position that is
> next_to_clean from the rx ring. After calling 'continue', another loop
> starts, 'first' is not updated and then will be passed to the receive
> function, which might cause the unexpected behavior as you said? So
> can this patch prevent such an issue from happening in this case?
>
> I'm not sure if I'm missing something.

No, these are good questions!

It really depends on whether a status descriptor can be received in the
middle of multi-buffer packet. Based on the existing code, I assumed it
can. Therefore, consider this case:

[valid_1st_packet][status_descriptor][valid_2nd_packet]

In this case you want to skip status_descriptor but keep the existing
logic that leads to:

    first = next_to_clean = valid_1st_packet

so then you can go and add valid_2nd_packet as a fragment to the first.

Ciao,
Alessandro