[PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Chenyuan Yang]

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 driver does not check for this NULL return value
in the XDP_TX case within efx_do_xdp(). While the efx_xdp_tx_buffers()
function has some defensive checks, passing a NULL xdpf can still lead
to undefined behavior when the function tries to access xdpf->len and
xdpf->data.

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: 1b698fa5d8ef ("xdp: Rename convert_to_xdp_frame in xdp_convert_buff_to_frame")
---
 drivers/net/ethernet/sfc/rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index ffca82207e47..6321ccd8c8fa 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -308,6 +308,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_free_rx_buffers(rx_queue, rx_buf, 1);
+			channel->n_rx_xdp_bad_drops++;
+			break;
+		}
+
 		err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
 		if (unlikely(err != 1)) {
 			efx_free_rx_buffers(rx_queue, rx_buf, 1);
-- 
2.34.1

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Paolo Abeni]

On 7/23/25 2:32 AM, 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.

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.

@Eduard: could you please have a look?

Thanks,

Paolo

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Edward Cree]

On 7/24/25 10:57, Paolo Abeni wrote:
> On 7/23/25 2:32 AM, 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.
> 
> 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.

Your reasoning seems plausible to me.
However, I think the error path ought to more closely follow the existing
 error cases in logging a ratelimited message and calling the tracepoint.
I think the cleanest way to do this would be:
	if (unlikely(!xdpf))
		err = -ENOBUFS;
	else
		err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
 so that it can make use of the existing failure path.
Adding the check to efx_xdp_tx_buffers() is also an option.

-ed

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Kunwu Chan]

On 2025/7/25 18:11, Edward Cree wrote:
> On 7/24/25 10:57, Paolo Abeni wrote:
>> On 7/23/25 2:32 AM, 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.
>> 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.
> Your reasoning seems plausible to me.
> However, I think the error path ought to more closely follow the existing
>   error cases in logging a ratelimited message and calling the tracepoint.
> I think the cleanest way to do this would be:
> 	if (unlikely(!xdpf))
> 		err = -ENOBUFS;
> 	else
> 		err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
>   so that it can make use of the existing failure path.
> Adding the check to efx_xdp_tx_buffers() is also an option.
>
> -ed
>
Hi Chenyuan,

THX for addressing this edge case. I concur with Edward's suggestion to 
integrate this with the existing error handling flow. This will ensure:
Consistent observability (ratelimited logs + tracepoints)
Centralized resource cleanup
Clear error type differentiation via -ENOBUFS

Proposed refinement:

diff
  case XDP_TX:
      /* Buffer ownership passes to tx on success. */
      xdpf = xdp_convert_buff_to_frame(&xdp);
+    if (unlikely(!xdpf)) {
+        err = -ENOBUFS;
+    } else {
+        err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
+    }

-    err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
      if (unlikely(err != 1)) {
          efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
          if (net_ratelimit())
              netif_err(efx, rx_err, efx->net_dev,
-                  "XDP TX failed (%d)\n", err);
+                  "XDP TX failed (%d)%s\n", err,
+                  err == -ENOBUFS ? " [frame conversion]" : "");
          channel->n_rx_xdp_bad_drops++;
-        trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
+        if (err != -ENOBUFS)
+            trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
      } else {
          channel->n_rx_xdp_tx++;
      }
      break;


-- Thanks, TAO. --- “Life finds a way.”

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Chenyuan Yang]

Thanks so much for your suggestion! I just submitted the v2 based on
your comments for this issue :)

On Fri, Jul 25, 2025 at 5:39 AM Kunwu Chan <kunwu.chan@linux.dev> wrote:
>
> On 2025/7/25 18:11, Edward Cree wrote:
> > On 7/24/25 10:57, Paolo Abeni wrote:
> >> On 7/23/25 2:32 AM, 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.
> >> 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.
> > Your reasoning seems plausible to me.
> > However, I think the error path ought to more closely follow the existing
> >   error cases in logging a ratelimited message and calling the tracepoint.
> > I think the cleanest way to do this would be:
> >       if (unlikely(!xdpf))
> >               err = -ENOBUFS;
> >       else
> >               err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
> >   so that it can make use of the existing failure path.
> > Adding the check to efx_xdp_tx_buffers() is also an option.
> >
> > -ed
> >
> Hi Chenyuan,
>
> THX for addressing this edge case. I concur with Edward's suggestion to
> integrate this with the existing error handling flow. This will ensure:
> Consistent observability (ratelimited logs + tracepoints)
> Centralized resource cleanup
> Clear error type differentiation via -ENOBUFS
>
> Proposed refinement:
>
> diff
>   case XDP_TX:
>       /* Buffer ownership passes to tx on success. */
>       xdpf = xdp_convert_buff_to_frame(&xdp);
> +    if (unlikely(!xdpf)) {
> +        err = -ENOBUFS;
> +    } else {
> +        err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
> +    }
>
> -    err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
>       if (unlikely(err != 1)) {
>           efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
>           if (net_ratelimit())
>               netif_err(efx, rx_err, efx->net_dev,
> -                  "XDP TX failed (%d)\n", err);
> +                  "XDP TX failed (%d)%s\n", err,
> +                  err == -ENOBUFS ? " [frame conversion]" : "");
>           channel->n_rx_xdp_bad_drops++;
> -        trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
> +        if (err != -ENOBUFS)
> +            trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
>       } else {
>           channel->n_rx_xdp_tx++;
>       }
>       break;
>
>
> -- Thanks, TAO. --- “Life finds a way.”

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Edward Cree]

On 25/07/2025 13:38, Kunwu Chan wrote:
> Proposed refinement:
...
>          if (net_ratelimit())
>              netif_err(efx, rx_err, efx->net_dev,
> -                  "XDP TX failed (%d)\n", err);
> +                  "XDP TX failed (%d)%s\n", err,
> +                  err == -ENOBUFS ? " [frame conversion]" : "");

Unnecessary, since efx_xdp_tx_buffers() never returns ENOBUFS.

>          channel->n_rx_xdp_bad_drops++;
> -        trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
> +        if (err != -ENOBUFS)
> +            trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);

Why prevent the tracepoint in this case??

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Kunwu Chan]

On 2025/7/28 22:28, Edward Cree wrote:
> On 25/07/2025 13:38, Kunwu Chan wrote:
>> Proposed refinement:
> ...
>>           if (net_ratelimit())
>>               netif_err(efx, rx_err, efx->net_dev,
>> -                  "XDP TX failed (%d)\n", err);
>> +                  "XDP TX failed (%d)%s\n", err,
>> +                  err == -ENOBUFS ? " [frame conversion]" : "");
> Unnecessary, since efx_xdp_tx_buffers() never returns ENOBUFS.

You're correct that efx_siena_xdp_tx_buffers() never returns -ENOBUFS,

and xdp_convert_buff_to_frame() returns NULL on failure.

Whether we need a log to distinguish where the errors come from?

  +   if (unlikely(!xdpf))

  +          err = -ENOBUFS;

  +   else

  +      err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);

  -    err = efx_siena_xdp_tx_buffers(efx, 1, &xdpf, true);
       if (unlikely(err != 1)) {
           efx_siena_free_rx_buffers(rx_queue, rx_buf, 1);
           if (net_ratelimit())
               netif_err(efx, rx_err, efx->net_dev,
-                  "XDP TX failed (%d)\n", err);
+                  "XDP TX failed (%d)%s\n", err,
+                  err == -ENOBUFS ? " [xdp_convert_buff_to_frame]" : 
"efx_siena_xdp_tx_buffers");

Just a personal thought.


>
>>           channel->n_rx_xdp_bad_drops++;
>> -        trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
>> +        if (err != -ENOBUFS)
>> +            trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
> Why prevent the tracepoint in this case??
You're correct, my mistake.

-- 
Thanks,
        TAO.
---
“Life finds a way.”

Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

[Jesper Dangaard Brouer]

On 25/07/2025 12.11, Edward Cree wrote:
> On 7/24/25 10:57, Paolo Abeni wrote:
>> On 7/23/25 2:32 AM, 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.
>>
>> 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.
> 
> Your reasoning seems plausible to me.

Hmm... have you actually tested that XDP/BPF can adjust headroom so much
that xdp_convert_buff_to_frame() function fails?

I really doubt this possible for BPF-progs to violate this.

The XDP BPF-prog can only adjust the headroom via the helpers
bpf_xdp_adjust_head() and bpf_xdp_adjust_meta().  These helpers reserve
room for sizeof(struct xdp_frame).

The tailroom can be adjusted via helper bpf_xdp_adjust_tail() and it
also reserve room for sizeof(struct skb_shared_info) such that BPF-progs
cannot get access to this area. See define for xdp_data_hard_end.

--Jesper