Re: [PATCH v2] sfc: handle NULL returned by xdp_convert_buff_to_frame()
From: Jesper Dangaard Brouer <hawk@kernel.org>
Date: 2025-07-31 09:23:30
Also in:
bpf
On 26/07/2025 21.56, 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 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.
The XDP_TX case is only for driver local frames/packets. And Edward says the sfc driver reserves both enough headroom and tailroom.
Fix by adding a proper NULL check in the XDP_TX case, following the suggestions of the developers.
In [V1] reply I question if this is possible - [v1] https://lore.kernel.org/all/a13646af-78f7-4ba7-9767-41d598222b1d@kernel.org/ (local) 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.
quoted hunk ↗ jump to hunk
Fixes: 1b698fa5d8ef ("xdp: Rename convert_to_xdp_frame in xdp_convert_buff_to_frame") Signed-off-by: Chenyuan Yang <redacted> Cc: Kunwu Chan <redacted> Cc: Edward Cree <redacted> --- drivers/net/ethernet/sfc/rx.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c index ffca82207e47..b56457c23f66 100644 --- a/drivers/net/ethernet/sfc/rx.c +++ b/drivers/net/ethernet/sfc/rx.c@@ -308,14 +308,20 @@ 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); - err = efx_xdp_tx_buffers(efx, 1, &xdpf, true); + if (unlikely(!xdpf)) + err = -ENOBUFS; + else + err = efx_xdp_tx_buffers(efx, 1, &xdpf, true); + if (unlikely(err != 1)) { efx_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++; }