| Message ID | 20250414183403.265943-1-sdl@nppct.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-netfront: handle NULL returned by xdp_convert_buff_to_frame() | expand |
On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: > get_page(pdata); Please notice this get_page() here. > xdpf = xdp_convert_buff_to_frame(xdp); > + if (unlikely(!xdpf)) { > + trace_xdp_exception(queue->info->netdev, prog, act); > + break; > + }
On 17.04.2025 03:58, Jakub Kicinski wrote: > On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >> get_page(pdata); > Please notice this get_page() here. > >> xdpf = xdp_convert_buff_to_frame(xdp); >> + if (unlikely(!xdpf)) { >> + trace_xdp_exception(queue->info->netdev, prog, act); >> + break; >> + } Do you mean that it would be better to move the get_page(pdata) call lower, after checking for NULL in xdpf, so that the reference count is only increased after a successful conversion?
On 17.04.25 09:00, Alexey wrote: > > On 17.04.2025 03:58, Jakub Kicinski wrote: >> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>> get_page(pdata); >> Please notice this get_page() here. >> >>> xdpf = xdp_convert_buff_to_frame(xdp); >>> + if (unlikely(!xdpf)) { >>> + trace_xdp_exception(queue->info->netdev, prog, act); >>> + break; >>> + } > Do you mean that it would be better to move the get_page(pdata) call lower, > after checking for NULL in xdpf, so that the reference count is only increased > after a successful conversion? I think the error handling here is generally broken (or at least very questionable). I suspect that in case of at least some errors the get_page() is leaking even without this new patch. In case I'm wrong a comment reasoning why there is no leak should be added. Juergen
On 17.04.2025 10:12, Jürgen Groß wrote: > On 17.04.25 09:00, Alexey wrote: >> >> On 17.04.2025 03:58, Jakub Kicinski wrote: >>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>> get_page(pdata); >>> Please notice this get_page() here. >>> >>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>> + if (unlikely(!xdpf)) { >>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>> + break; >>>> + } >> Do you mean that it would be better to move the get_page(pdata) call >> lower, >> after checking for NULL in xdpf, so that the reference count is only >> increased >> after a successful conversion? > > I think the error handling here is generally broken (or at least very > questionable). > > I suspect that in case of at least some errors the get_page() is leaking > even without this new patch. > > In case I'm wrong a comment reasoning why there is no leak should be > added. > > > Juergen I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()
On 17.04.25 10:45, Alexey wrote: > > On 17.04.2025 10:12, Jürgen Groß wrote: >> On 17.04.25 09:00, Alexey wrote: >>> >>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>> get_page(pdata); >>>> Please notice this get_page() here. >>>> >>>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>>> + if (unlikely(!xdpf)) { >>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>> + break; >>>>> + } >>> Do you mean that it would be better to move the get_page(pdata) call lower, >>> after checking for NULL in xdpf, so that the reference count is only increased >>> after a successful conversion? >> >> I think the error handling here is generally broken (or at least very >> questionable). >> >> I suspect that in case of at least some errors the get_page() is leaking >> even without this new patch. >> >> In case I'm wrong a comment reasoning why there is no leak should be >> added. >> >> >> Juergen > > I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() Agreed. But what if xennet_xdp_xmit() returns an error < 0? In this case xdp_return_frame_rx_napi() won't be called. Juergen
On 17.04.2025 11:51, Juergen Gross wrote: > On 17.04.25 10:45, Alexey wrote: >> >> On 17.04.2025 10:12, Jürgen Groß wrote: >>> On 17.04.25 09:00, Alexey wrote: >>>> >>>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>>> get_page(pdata); >>>>> Please notice this get_page() here. >>>>> >>>>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>>>> + if (unlikely(!xdpf)) { >>>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>>> + break; >>>>>> + } >>>> Do you mean that it would be better to move the get_page(pdata) >>>> call lower, >>>> after checking for NULL in xdpf, so that the reference count is >>>> only increased >>>> after a successful conversion? >>> >>> I think the error handling here is generally broken (or at least very >>> questionable). >>> >>> I suspect that in case of at least some errors the get_page() is >>> leaking >>> even without this new patch. >>> >>> In case I'm wrong a comment reasoning why there is no leak should be >>> added. >>> >>> >>> Juergen >> >> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() > > Agreed. But what if xennet_xdp_xmit() returns an error < 0? > > In this case xdp_return_frame_rx_napi() won't be called. > > > Juergen Agreed. There is no explicit freed pdata in the calling function xennet_get_responses(). Without this, the page referenced by pdata could be leaked. I suggest: case XDP_TX: -get_page(pdata); xdpf = xdp_convert_buff_to_frame(xdp); +if (unlikely(!xdpf)) { +trace_xdp_exception(queue->info->netdev, prog, act); +break; +} +get_page(pdata); err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0); if (unlikely(!err)) xdp_return_frame_rx_napi(xdpf); -else if (unlikely(err < 0)) +else if (unlikely(err < 0)) { trace_xdp_exception(queue->info->netdev, prog, act); +xdp_return_frame_rx_napi(xdpf); +} break; case XDP_REDIRECT: get_page(pdata); err = xdp_do_redirect(queue->info->netdev, xdp, prog); *need_xdp_flush = true; -if (unlikely(err)) +if (unlikely(err)) { trace_xdp_exception(queue->info->netdev, prog, act); +__xdp_return(page_address(pdata), &xdp->mem, true, xdp); +} break;
On 17.04.25 12:06, Alexey wrote: > > On 17.04.2025 11:51, Juergen Gross wrote: >> On 17.04.25 10:45, Alexey wrote: >>> >>> On 17.04.2025 10:12, Jürgen Groß wrote: >>>> On 17.04.25 09:00, Alexey wrote: >>>>> >>>>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>>>> get_page(pdata); >>>>>> Please notice this get_page() here. >>>>>> >>>>>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>>>>> + if (unlikely(!xdpf)) { >>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>>>> + break; >>>>>>> + } >>>>> Do you mean that it would be better to move the get_page(pdata) call lower, >>>>> after checking for NULL in xdpf, so that the reference count is only increased >>>>> after a successful conversion? >>>> >>>> I think the error handling here is generally broken (or at least very >>>> questionable). >>>> >>>> I suspect that in case of at least some errors the get_page() is leaking >>>> even without this new patch. >>>> >>>> In case I'm wrong a comment reasoning why there is no leak should be >>>> added. >>>> >>>> >>>> Juergen >>> >>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() >> >> Agreed. But what if xennet_xdp_xmit() returns an error < 0? >> >> In this case xdp_return_frame_rx_napi() won't be called. >> >> >> Juergen > > Agreed. There is no explicit freed pdata in the calling function > xennet_get_responses(). Without this, the page referenced by pdata > could be leaked. > > I suggest: > > case XDP_TX: > - get_page(pdata); > xdpf = xdp_convert_buff_to_frame(xdp); > + if (unlikely(!xdpf)) { > + trace_xdp_exception(queue->info->netdev, prog, act); > + break; > + } > + get_page(pdata); > err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0); > if (unlikely(!err)) > xdp_return_frame_rx_napi(xdpf); > - else if (unlikely(err < 0)) > + else if (unlikely(err < 0)) { > trace_xdp_exception(queue->info->netdev, prog, act); > + xdp_return_frame_rx_napi(xdpf); > + } Could you please merge the two if () blocks, as they share the call of xdp_return_frame_rx_napi() now? Something like: if (unlikely(err <= 0)) { if (err < 0) trace_xdp_exception(queue->info->netdev, prog, act); xdp_return_frame_rx_napi(xdpf); } > break; > case XDP_REDIRECT: > get_page(pdata); > err = xdp_do_redirect(queue->info->netdev, xdp, prog); > *need_xdp_flush = true; > - if (unlikely(err)) > + if (unlikely(err)) { > trace_xdp_exception(queue->info->netdev, prog, act); > + __xdp_return(page_address(pdata), &xdp->mem, true, xdp); > + } > break; Juergen P.S.: please don't use HTML in emails
On 17.04.2025 13:23, Jürgen Groß wrote: > On 17.04.25 12:06, Alexey wrote: >> >> On 17.04.2025 11:51, Juergen Gross wrote: >>> On 17.04.25 10:45, Alexey wrote: >>>> >>>> On 17.04.2025 10:12, Jürgen Groß wrote: >>>>> On 17.04.25 09:00, Alexey wrote: >>>>>> >>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>>>>> get_page(pdata); >>>>>>> Please notice this get_page() here. >>>>>>> >>>>>>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>>>>>> + if (unlikely(!xdpf)) { >>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>>>>> + break; >>>>>>>> + } >>>>>> Do you mean that it would be better to move the get_page(pdata) >>>>>> call lower, >>>>>> after checking for NULL in xdpf, so that the reference count is >>>>>> only increased >>>>>> after a successful conversion? >>>>> >>>>> I think the error handling here is generally broken (or at least very >>>>> questionable). >>>>> >>>>> I suspect that in case of at least some errors the get_page() is >>>>> leaking >>>>> even without this new patch. >>>>> >>>>> In case I'm wrong a comment reasoning why there is no leak should be >>>>> added. >>>>> >>>>> >>>>> Juergen >>>> >>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() >>> >>> Agreed. But what if xennet_xdp_xmit() returns an error < 0? >>> >>> In this case xdp_return_frame_rx_napi() won't be called. >>> >>> >>> Juergen >> >> Agreed. There is no explicit freed pdata in the calling function >> xennet_get_responses(). Without this, the page referenced by pdata >> could be leaked. >> >> I suggest: > > Could you please merge the two if () blocks, as they share the > call of xdp_return_frame_rx_napi() now? Something like: > > if (unlikely(err <= 0)) { > if (err < 0) > trace_xdp_exception(queue->info->netdev, prog, act); > xdp_return_frame_rx_napi(xdpf); > } > > Juergen > > P.S.: please don't use HTML in emails I can't do this because xennet_xdp_xmit() can return a value > 0
On 17.04.25 13:19, Alexey wrote: > > On 17.04.2025 13:23, Jürgen Groß wrote: >> On 17.04.25 12:06, Alexey wrote: >>> >>> On 17.04.2025 11:51, Juergen Gross wrote: >>>> On 17.04.25 10:45, Alexey wrote: >>>>> >>>>> On 17.04.2025 10:12, Jürgen Groß wrote: >>>>>> On 17.04.25 09:00, Alexey wrote: >>>>>>> >>>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote: >>>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote: >>>>>>>>> get_page(pdata); >>>>>>>> Please notice this get_page() here. >>>>>>>> >>>>>>>>> xdpf = xdp_convert_buff_to_frame(xdp); >>>>>>>>> + if (unlikely(!xdpf)) { >>>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act); >>>>>>>>> + break; >>>>>>>>> + } >>>>>>> Do you mean that it would be better to move the get_page(pdata) call lower, >>>>>>> after checking for NULL in xdpf, so that the reference count is only >>>>>>> increased >>>>>>> after a successful conversion? >>>>>> >>>>>> I think the error handling here is generally broken (or at least very >>>>>> questionable). >>>>>> >>>>>> I suspect that in case of at least some errors the get_page() is leaking >>>>>> even without this new patch. >>>>>> >>>>>> In case I'm wrong a comment reasoning why there is no leak should be >>>>>> added. >>>>>> >>>>>> >>>>>> Juergen >>>>> >>>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return() >>>> >>>> Agreed. But what if xennet_xdp_xmit() returns an error < 0? >>>> >>>> In this case xdp_return_frame_rx_napi() won't be called. >>>> >>>> >>>> Juergen >>> >>> Agreed. There is no explicit freed pdata in the calling function >>> xennet_get_responses(). Without this, the page referenced by pdata >>> could be leaked. >>> >>> I suggest: >> >> Could you please merge the two if () blocks, as they share the >> call of xdp_return_frame_rx_napi() now? Something like: >> >> if (unlikely(err <= 0)) { >> if (err < 0) >> trace_xdp_exception(queue->info->netdev, prog, act); >> xdp_return_frame_rx_napi(xdpf); >> } >> >> Juergen >> >> P.S.: please don't use HTML in emails > > I can't do this because xennet_xdp_xmit() can return a value > 0 Yes, this is what the "if (unlikely(err <= 0))" is for. Juergen
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index 8425226c09f0..e99561de3cda 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -987,6 +987,10 @@ static u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata, case XDP_TX: get_page(pdata); xdpf = xdp_convert_buff_to_frame(xdp); + if (unlikely(!xdpf)) { + trace_xdp_exception(queue->info->netdev, prog, act); + break; + } err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0); if (unlikely(!err)) xdp_return_frame_rx_napi(xdpf);
The function xdp_convert_buff_to_frame() may return NULL if it fails to correctly convert the XDP buffer into an XDP frame due to memory constraints, internal errors, or invalid data. Failing to check for NULL may lead to a NULL pointer dereference if the result is used later in processing, potentially causing crashes, data corruption, or undefined behavior. Found by Linux Verification Center (linuxtesting.org) with SVACE. Cc: stable@vger.kernel.org # v5.9+ Fixes: 6c5aa6fc4def ("xen networking: add basic XDP support for xen-netfront") Signed-off-by: Alexey Nepomnyashih <sdl@nppct.ru> --- drivers/net/xen-netfront.c | 4 ++++ 1 file changed, 4 insertions(+)