>-----Original Message-----
>From: Chenyuan Yang <chenyuan0y@xxxxxxxxx>
>Sent: Sunday, July 27, 2025 1:51 AM
>To: Paolo Abeni <pabeni@xxxxxxxxxx>
>Cc: Geethasowjanya Akula <gakula@xxxxxxxxxxx>; Sunil Kovvuri Goutham
><sgoutham@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
><sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>; Bharat
>Bhushan <bbhushan2@xxxxxxxxxxx>; andrew+netdev@xxxxxxx;
>davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
>ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; hawk@xxxxxxxxxx;
>john.fastabend@xxxxxxxxx; sdf@xxxxxxxxxxx; Suman Ghosh
><sumang@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx;
>zzjas98@xxxxxxxxx
>Subject: Re: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by
>xdp_convert_buff_to_frame()
>
>On Thu, Jul 24, 2025 at 3:11 AM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>>
>> On 7/23/25 5:36 AM, Geethasowjanya Akula wrote:
>> >> -----Original Message-----
>> >> From: Geethasowjanya Akula
>> >> Sent: Wednesday, July 23, 2025 8:59 AM
>> >> To: Chenyuan Yang <chenyuan0y@xxxxxxxxx>; Sunil Kovvuri Goutham
>> >> <sgoutham@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
>> >> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>;
>> >> Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; andrew+netdev@xxxxxxx;
>> >> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
>> >> pabeni@xxxxxxxxxx; ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx;
>> >> hawk@xxxxxxxxxx; john.fastabend@xxxxxxxxx; sdf@xxxxxxxxxxx; Suman
>> >> Ghosh <sumang@xxxxxxxxxxx>
>> >> Cc: netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx; zzjas98@xxxxxxxxx
>> >> Subject: RE: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by
>> >> xdp_convert_buff_to_frame()
>> >>
>> >>
>> >>
>> >>> -----Original Message-----
>> >>> From: Chenyuan Yang <chenyuan0y@xxxxxxxxx>
>> >>> Sent: Wednesday, July 23, 2025 6:03 AM
>> >>> To: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya
>> >> Akula
>> >>> <gakula@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
>> >>> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>;
>> >>> Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; andrew+netdev@xxxxxxx;
>> >> davem@xxxxxxxxxxxxx;
>> >>> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
>> >>> ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; hawk@xxxxxxxxxx;
>> >>> john.fastabend@xxxxxxxxx; sdf@xxxxxxxxxxx; Suman Ghosh
>> >>> <sumang@xxxxxxxxxxx>
>> >>> Cc: netdev@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx;
>> >>> zzjas98@xxxxxxxxx; Chenyuan Yang <chenyuan0y@xxxxxxxxx>
>> >>> Subject: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by
>> >>> xdp_convert_buff_to_frame()
>> >>>
>> >>> 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 otx2 driver does not check for this NULL return
>> >>> value in two critical paths within otx2_xdp_rcv_pkt_handler():
>> >>>
>> >>> 1. XDP_TX case: Passes potentially NULL xdpf to
>> >>> otx2_xdp_sq_append_pkt()
>> >> 2.
>> >>> XDP_REDIRECT error path: Calls xdp_return_frame() with potentially
>> >>> NULL
>> >>>
>> >>> This can lead to kernel crashes due to NULL pointer dereference.
>> >>>
>> >>> Fix by adding proper NULL checks in both paths. For XDP_TX, return
>> >>> false to indicate packet should be dropped. For XDP_REDIRECT error
>> >>> path, only call
>> >>> xdp_return_frame() if conversion succeeded, otherwise manually
>> >>> free the page.
>> >>>
>> >>> Please correct me if any error path is incorrect.
>> >>>
>> >>> This is similar to the commit cc3628dcd851
>> >>> ("xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()").
>> >>>
>> >>> Signed-off-by: Chenyuan Yang <chenyuan0y@xxxxxxxxx>
>> >>> Fixes: 94c80f748873 ("octeontx2-pf: use xdp_return_frame() to free
>> >>> xdp
>> >>> buffers")
>> >>> ---
>> >>> drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 8
>> >>> +++++++-
>> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git
>> >>> a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>> >>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>> >>> index 99ace381cc78..0c4c050b174a 100644
>> >>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>> >>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>> >>> @@ -1534,6 +1534,9 @@ static bool otx2_xdp_rcv_pkt_handler(struct
>> >>> otx2_nic *pfvf,
>> >>> qidx += pfvf->hw.tx_queues;
>> >>> cq->pool_ptrs++;
>> >>> xdpf = xdp_convert_buff_to_frame(&xdp);
>> >>> + if (unlikely(!xdpf))
>> >>> + return false;
>> >>> +
>> >>> return otx2_xdp_sq_append_pkt(pfvf, xdpf,
>> >>> cqe->sg.seg_addr,
>> >>> cqe->sg.seg_size, @@
>> >>> -1558,7 +1561,10 @@ static bool otx2_xdp_rcv_pkt_handler(struct
>> >>> otx2_nic *pfvf,
>> >>> otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
>> >>> DMA_FROM_DEVICE);
>> >>> xdpf = xdp_convert_buff_to_frame(&xdp);
>> >>> - xdp_return_frame(xdpf);
>> >>> + if (likely(xdpf))
>> >>> + xdp_return_frame(xdpf);
>> >>> + else
>> >>> + put_page(page);
>> >> Thanks for the fix. Given that the page is already freed, returning
>> >> true in this case makes sense.
>> > This change might not be directly related to the current patch,
>> > though. You can either include it here or we can submit a follow-up patch
>to address it.
>>
>> If I read correctly, returning false as the current patch is doing,
>> will make the later code in otx2_rcv_pkt_handler() unconditionally use
>> the just freed page.
>>
>> I think returning true after put_page() is strictly necessary.
>
>Thanks for the review and for catching that issue. You're right, returning false
>would cause a use-after-free, as the caller would proceed to use the already
>freed page.
>
>I've updated the patch to return true in the XDP_TX failure case. I also adjusted
Returning true on XDP_TX failure requires the page to be freed. Otherwise, it should
return false so the packet can be handled by otx2_rcv_pkt_handler.
>the XDP_REDIRECT error path to do the same after calling put_page(),
>preventing a fall-through.
>
>Does the updated patch below look correct? If so, I'll send out a formal v2.
>
>---
> drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>index 99ace381cc78..4e1b9a3f6e51 100644
>--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>@@ -1534,6 +1534,9 @@ static bool otx2_xdp_rcv_pkt_handler(struct
>otx2_nic *pfvf,
> qidx += pfvf->hw.tx_queues;
> cq->pool_ptrs++;
> xdpf = xdp_convert_buff_to_frame(&xdp);
>+ if (unlikely(!xdpf))
>+ return true;
>+
> return otx2_xdp_sq_append_pkt(pfvf, xdpf,
> cqe->sg.seg_addr,
> cqe->sg.seg_size,
>@@ -1558,7 +1561,12 @@ static bool otx2_xdp_rcv_pkt_handler(struct
>otx2_nic *pfvf,
> otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
> DMA_FROM_DEVICE);
> xdpf = xdp_convert_buff_to_frame(&xdp);
>- xdp_return_frame(xdpf);
>+ if (likely(xdpf)) {
>+ xdp_return_frame(xdpf);
>+ } else {
>+ put_page(page);
>+ return true;
>+ }
> break;
> default:
> bpf_warn_invalid_xdp_action(pfvf->netdev, prog, act);
>---
>
>
>> /P
>>
