netdev - Re: [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_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] [day] [month] [year] [list]

Message-ID: <20250725093934.GB1367887@horms.kernel.org>
Date: Fri, 25 Jul 2025 10:39:34 +0100
From: Simon Horman <horms@...nel.org>
To: Chenyuan Yang <chenyuan0y@...il.com>
Cc: ecree.xilinx@...il.com, andrew+netdev@...n.ch, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	ast@...nel.org, daniel@...earbox.net, hawk@...nel.org,
	john.fastabend@...il.com, sdf@...ichev.me, martinh@...inx.com,
	netdev@...r.kernel.org, linux-net-drivers@....com,
	bpf@...r.kernel.org, zzjas98@...il.com
Subject: Re: [PATCH] sfc: siena: handle NULL returned by
 xdp_convert_buff_to_frame()

On Fri, Jul 25, 2025 at 10:34:01AM +0100, Simon Horman wrote:
> On Tue, Jul 22, 2025 at 07:32:30PM -0500, Chenyuan Yang wrote:
> > The xdp_convert_buff_to_frame() function can return NULL when there is
> > insufficient headroom in the buffer to store the xdp_frame structure
> > or when the driver didn't reserve enough tailroom for skb_shared_info.
> > 
> > Currently, the sfc siena driver does not check for this NULL return value
> > in the XDP_TX case within efx_do_xdp().
> > 
> > Fix by adding a proper NULL check in the XDP_TX case. If conversion
> > fails, free the RX buffer and increment the bad drops counter, following
> > the same pattern used for other XDP error conditions in this driver.
> > 
> > Signed-off-by: Chenyuan Yang <chenyuan0y@...il.com>
> > Fixes: d48523cb88e0 ("sfc: Copy shared files needed for Siena (part 2)")
> > ---
> >  drivers/net/ethernet/sfc/siena/rx.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/sfc/siena/rx.c b/drivers/net/ethernet/sfc/siena/rx.c
> > index 98d3c0743c0f..65f6daad3168 100644
> > --- a/drivers/net/ethernet/sfc/siena/rx.c
> > +++ b/drivers/net/ethernet/sfc/siena/rx.c
> > @@ -310,6 +310,12 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,
> >  	case XDP_TX:
> >  		/* Buffer ownership passes to tx on success. */
> >  		xdpf = xdp_convert_buff_to_frame(&xdp);
> > +		if (unlikely(!xdpf)) {
> > +			efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
> > +			channel->n_rx_xdp_bad_drops++;
> > +			break;
> > +		}
> 
> I see the incidence of this cleanup code multiplying in this function.
> I would be nice to follow-up by consolidating that - I'd suggest
> in the form of a goto label at the end of the function.
> 
> But as a fix for net this minimal patch seems appropriate to me.
> 
> Reviewed-by: Simon Horman <horms@...nel.org>

Oops, a few moments later I now see that Paolo felt otherwise
for a similar patch [1].

  "AFAIC the sfc driver reserves both enough headroom and tailroom, but
   this is after ebpf run, which in turn could consume enough headroom to
   cause a failure, so I think this makes sense."

Let's go with his advice and and for a review by Edward.

[1] Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()
    https://lore.kernel.org/all/045d1ff5-bb20-481d-a067-0a42345ab83d@redhat.com/