* [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_to_frame()
@ 2025-07-23 0:32 Chenyuan Yang
2025-07-25 9:34 ` Simon Horman
0 siblings, 1 reply; 3+ messages in thread
From: Chenyuan Yang @ 2025-07-23 0:32 UTC (permalink / raw)
To: ecree.xilinx, andrew+netdev, davem, edumazet, kuba, pabeni, ast,
daniel, hawk, john.fastabend, sdf, chenyuan0y, martinh
Cc: netdev, linux-net-drivers, bpf, zzjas98
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@gmail.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;
+ }
+
err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
if (unlikely(err != 1)) {
efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_to_frame()
2025-07-23 0:32 [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_to_frame() Chenyuan Yang
@ 2025-07-25 9:34 ` Simon Horman
2025-07-25 9:39 ` Simon Horman
0 siblings, 1 reply; 3+ messages in thread
From: Simon Horman @ 2025-07-25 9:34 UTC (permalink / raw)
To: Chenyuan Yang
Cc: ecree.xilinx, andrew+netdev, davem, edumazet, kuba, pabeni, ast,
daniel, hawk, john.fastabend, sdf, martinh, netdev,
linux-net-drivers, bpf, zzjas98
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@gmail.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@kernel.org>
> +
> err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
> if (unlikely(err != 1)) {
> efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_to_frame()
2025-07-25 9:34 ` Simon Horman
@ 2025-07-25 9:39 ` Simon Horman
0 siblings, 0 replies; 3+ messages in thread
From: Simon Horman @ 2025-07-25 9:39 UTC (permalink / raw)
To: Chenyuan Yang
Cc: ecree.xilinx, andrew+netdev, davem, edumazet, kuba, pabeni, ast,
daniel, hawk, john.fastabend, sdf, martinh, netdev,
linux-net-drivers, bpf, zzjas98
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@gmail.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@kernel.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/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-07-25 9:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-23 0:32 [PATCH] sfc: siena: handle NULL returned by xdp_convert_buff_to_frame() Chenyuan Yang
2025-07-25 9:34 ` Simon Horman
2025-07-25 9:39 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox