netdev - Re: [PATCH] sfc: 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: <CALGdzuquTFAgnKyPKBs2v4YwaPafzkVyt7qpXG3H0TKYj0-zPw@mail.gmail.com>
Date: Sat, 26 Jul 2025 12:56:48 -0700
From: Chenyuan Yang <chenyuan0y@...il.com>
To: Kunwu Chan <kunwu.chan@...ux.dev>
Cc: Edward Cree <ecree@....com>, Paolo Abeni <pabeni@...hat.com>, ecree.xilinx@...il.com, 
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, ast@...nel.org, daniel@...earbox.net, hawk@...nel.org, 
	john.fastabend@...il.com, sdf@...ichev.me, lorenzo@...nel.org, 
	netdev@...r.kernel.org, linux-net-drivers@....com, bpf@...r.kernel.org, 
	zzjas98@...il.com
Subject: Re: [PATCH] sfc: handle NULL returned by xdp_convert_buff_to_frame()

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@...ux.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.”