Re: [PATCH net 1/9] xsk: fix list corruption in xsk_buff_add_frag()

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Nov 03, 2026 at 01:32:29PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 0 · Low: 1 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] At the reviewed commit, the new `xdp == head` rejection in
>   xsk_buff_add_frag() (include/net/xdp_sock_drv.h:166-168) runs after…
> - [High] At the reviewed commit, the new rejection in xsk_buff_add_frag()
>   fails the same way every time it sees the same input.
> - [Low] The kernel-doc for libeth_xsk_buff_add_frag()
>   (drivers/net/ethernet/intel/libeth/xsk.c:50-51) says it returns '%NULL…
> 
> Pre-existing issues:
> - [Low] When xsk_buff_add_frag() fails, ice_clean_rx_irq_zc() frees the
>   partial frame and sets first = NULL but does not skip ahead to EOP.
> 

Note: I will be trimming away all the issues where beloved bot says 'a
later patch in the series seems to fix this'. This is really annoying :<

> --
> 
> xsk: fix list corruption in xsk_buff_add_frag()
> 
> This patch changes xsk_buff_add_frag() to reject a frag buffer that is the
> frame head or that is already linked on pool->xskb_list or
> pool->free_list. In aligned mode this happens when user space posts the
> same fill ring address twice. The goal is to prevent list corruption and
> the resulting NULL pointer dereference in xp_free().
> 
> > diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> > index d94aeb506379c..29965e3695983 100644
> > --- a/include/net/xdp_sock_drv.h
> > +++ b/include/net/xdp_sock_drv.h
> > @@ -160,15 +160,18 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
> >  static inline bool xsk_buff_add_frag(struct xdp_buff *head,
> >  				     struct xdp_buff *xdp)
> >  {
> > +	struct xdp_buff_xsk *frag = container_of(xdp, struct xdp_buff_xsk, xdp);
> >  	const void *data = xdp->data;
> > -	struct xdp_buff_xsk *frag;
> > +
> > +	/* Duplicate fill ring entries map to the same xskb in aligned mode */
> > +	if (unlikely(xdp == head || !list_empty(&frag->list_node)))
> > +		return false;
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch, but
> this check makes the caller failure paths reachable through duplicate fill
> ring entries.
> 
> In ice_clean_rx_irq_zc(), a failed xsk_buff_add_frag() frees the partial
> frame and sets first to NULL, but the remaining descriptors up to EOP are
> not skipped:
> 
>     } else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
>         xsk_buff_free(first);
>         first = NULL;
>     }
> 
> Won't the rest of the hardware frame then be assembled and delivered as a
> new frame? idpf_xskrq_poll() does the same when libeth_xsk_process_buff()
> returns NULL.
> 
> The i40e patch later in this series calls this intended ("The rest of the
> dropped frame is received as a new frame"). It also does not appear to
> corrupt kernel memory, so it may be acceptable as is.

Hm, yeah seems like frame should be drained up to EOP...it's best to do on
driver side.

Clashiko 1:2 Maciej

as two issues were just bonkers, they are fixed later.

> 
> [Severity: Low]
> This isn't a bug, but the kernel-doc for libeth_xsk_buff_add_frag() in
> drivers/net/ethernet/intel/libeth/xsk.c still lists running out of frag
> space as the only failure reason:
> 
>  * Return: main buffer with attached frag on success, %NULL on error (no space
>  * for a new frag).
> 
> With these new checks, it also returns NULL when the frag is the head, or
> when it is already on pool->xskb_list or pool->free_list. Should that
> comment be updated too? The text is unchanged at the end of the series.
> 
> >  
> >  	if (!__xdp_buff_add_frag(head, virt_to_netmem(data),
> >  				 offset_in_page(data), xdp->data_end - data,
> >  				 xdp->frame_sz, false))
> >  		return false;
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com