octeon_ep_vf: add NULL check for napi_build_skb()
PATCH 1/1 octeon_ep_vf: add NULL check for napi_build_skb()
napi_build_skb() can return NULL on allocation failure. In __octep_vf_oq_process_rx(), the result is used directly without a NULL check in both the single-buffer and multi-fragment paths, leading to a NULL pointer dereference.
Add NULL checks after both napi_build_skb() calls, properly advancing descriptors and consuming remaining fragments on failure.
--- .../marvell/octeon_ep_vf/octep_vf_rx.c | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c +33 -1
@@ -409,10 +409,18 @@ static int __octep_vf_oq_process_rx(struct octep_vf_device *oct, data_offset =OCTEP_VF_OQ_RESP_HW_SIZE ; rx_ol_flags =0 ; } - rx_bytes += buff_info->len;if (buff_info->len <= oq->max_single_buffer_size) { skb =napi_build_skb ((void *)resp_hw,PAGE_SIZE ); +if (!skb) { + oq->stats->alloc_failures++; + read_idx++; + desc_used++; +if (read_idx == oq->max_count) + read_idx =0 ; +continue ; + } + rx_bytes += buff_info->len;skb_reserve (skb, data_offset);skb_put (skb, buff_info->len); read_idx++;
@@ -424,6 +432,30 @@ static int __octep_vf_oq_process_rx(struct octep_vf_device *oct,u16 data_len; skb =napi_build_skb ((void *)resp_hw,PAGE_SIZE ); +if (!skb) { + oq->stats->alloc_failures++; + read_idx++; + desc_used++; +if (read_idx == oq->max_count) + read_idx =0 ; + data_len = buff_info->len - oq->max_single_buffer_size; +while (data_len) { +dma_unmap_page (oq->dev, oq->desc_ring[read_idx].buffer_ptr, +PAGE_SIZE ,DMA_FROM_DEVICE ); + buff_info = (struct octep_vf_rx_buffer *)&oq->buff_info[read_idx]; + buff_info->page =NULL ; +if (data_len < oq->buffer_size) + data_len =0 ; +else + data_len -= oq->buffer_size; + read_idx++; + desc_used++; +if (read_idx == oq->max_count) + read_idx =0 ; + } +continue ; + } + rx_bytes += buff_info->len;skb_reserve (skb, data_offset);/* Head fragment includes response header(s); * subsequent fragments contains only data.
Signature · 1 line
2.53.0
Simon Horman
reviewerQuoted 11 lines · David Carlier
napi_build_skb()can return NULL on allocation failure. In__octep_vf_oq_process_rx(), the result is used directly without a NULL check in both the single-buffer and multi-fragment paths, leading to a NULL pointer dereference.Add NULL checks after both
napi_build_skb()calls, properly advancing descriptors and consuming remaining fragments on failure.Fixes 1cd3b407977c ("octeon_ep_vf: add Tx/Rx processing and interrupt support")Signed-off-by David Carlier <[email protected]>
Hi David,
I appreciate that this is on the fast path, and thus I expect it is performance critical. But this patch largely duplicates code already present in the same function. Would it be possible refactor things a bit - e.g. using helpers - to make the change a bit cleaner while not hurting performance?
If so, I'd suggest splitting patch(es) that refactor the code from the patch that fixes the bug.
...
Hi Simon,
Quoted 22 lines · Simon Horman
Quoted 11 lines · David Carlier
napi_build_skb()can return NULL on allocation failure. In__octep_vf_oq_process_rx(), the result is used directly without a NULL check in both the single-buffer and multi-fragment paths, leading to a NULL pointer dereference.Add NULL checks after both
napi_build_skb()calls, properly advancing descriptors and consuming remaining fragments on failure.Fixes 1cd3b407977c ("octeon_ep_vf: add Tx/Rx processing and interrupt support")Signed-off-by David Carlier <[email protected]>Hi David,
I appreciate that this is on the fast path, and thus I expect it is performance critical. But this patch largely duplicates code already present in the same function. Would it be possible refactor things a bit - e.g. using helpers - to make the change a bit cleaner while not hurting performance?
Quoted 4 lines
If so, I'd suggest splitting patch(es) that refactor the code from the patch that fixes the bug.
...
Yes, valid points, I'll submit the v2 tomorrow. Cheers !