| Message ID | 20240930053328.9618-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] octeon_ep: Add SKB allocation failures handling in __octep_oq_process_rx() | expand |
On Mon, Sep 30, 2024 at 08:33:28AM +0300, Aleksandr Mishin wrote: > build_skb() returns NULL in case of a memory allocation failure so handle > it inside __octep_oq_process_rx() to avoid NULL pointer dereference. > > __octep_oq_process_rx() is called during NAPI polling by the driver. If > skb allocation fails, keep on pulling packets out of the Rx DMA queue: we > shouldn't break the polling immediately and thus falsely indicate to the > octep_napi_poll() that the Rx pressure is going down. As there is no > associated skb in this case, don't process the packets and don't push them > up the network stack - they are skipped. > > The common code with skb and some index manipulations is extracted to make > the fix more readable and avoid code duplication. Also helper function is > implemented to unmmap/flush all the fragment buffers used by the dropped > packet. 'alloc_failures' counter is incremented to mark the skb allocation > error in driver statistics. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support") > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Suggested-by: Simon Horman <horms@kernel.org> > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > A similar situation is present in the __octep_vf_oq_process_rx() of the > Octeon VF driver. First we want to try the fix on __octep_oq_process_rx(). One step at a time :) > Compile tested only. > > v3: > - Implement helper which frees current packet resources and increase > index and descriptor as suggested by Simon > (https://lore.kernel.org/all/20240919134812.GB1571683@kernel.org/) > - Optimize helper as suggested by Paolo > (https://lore.kernel.org/all/b9ae8575-f903-425f-aa42-0c2a7605aa94@redhat.com/) Thanks for the revision. This version looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> > v2: https://lore.kernel.org/all/20240916060212.12393-1-amishin@t-argos.ru/ > - Implement helper instead of adding multiple checks for '!skb' and > remove 'rx_bytes' increasing in case of packet dropping as suggested > by Paolo > (https://lore.kernel.org/all/ba514498-3706-413b-a09f-f577861eef28@redhat.com/) > v1: https://lore.kernel.org/all/20240906063907.9591-1-amishin@t-argos.ru/ > > .../net/ethernet/marvell/octeon_ep/octep_rx.c | 82 +++++++++++++------ > 1 file changed, 59 insertions(+), 23 deletions(-) ...
On 9/30/24 07:33, Aleksandr Mishin wrote: > build_skb() returns NULL in case of a memory allocation failure so handle > it inside __octep_oq_process_rx() to avoid NULL pointer dereference. > > __octep_oq_process_rx() is called during NAPI polling by the driver. If > skb allocation fails, keep on pulling packets out of the Rx DMA queue: we > shouldn't break the polling immediately and thus falsely indicate to the > octep_napi_poll() that the Rx pressure is going down. As there is no > associated skb in this case, don't process the packets and don't push them > up the network stack - they are skipped. > > The common code with skb and some index manipulations is extracted to make > the fix more readable and avoid code duplication. Also helper function is > implemented to unmmap/flush all the fragment buffers used by the dropped > packet. 'alloc_failures' counter is incremented to mark the skb allocation > error in driver statistics. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support") > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Suggested-by: Simon Horman <horms@kernel.org> > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > A similar situation is present in the __octep_vf_oq_process_rx() of the > Octeon VF driver. First we want to try the fix on __octep_oq_process_rx(). > > Compile tested only. @Marvel folks: it would be great if you could test this patch and report here, otherwise this is going to be merged with build test only... Thanks, Paolo
On Mon, 30 Sep 2024 08:33:28 +0300 Aleksandr Mishin wrote: > build_skb() returns NULL in case of a memory allocation failure so handle > it inside __octep_oq_process_rx() to avoid NULL pointer dereference. > > __octep_oq_process_rx() is called during NAPI polling by the driver. If > skb allocation fails, keep on pulling packets out of the Rx DMA queue: we > shouldn't break the polling immediately and thus falsely indicate to the > octep_napi_poll() that the Rx pressure is going down. As there is no > associated skb in this case, don't process the packets and don't push them > up the network stack - they are skipped. > > The common code with skb and some index manipulations is extracted to make > the fix more readable and avoid code duplication. Also helper function is > implemented to unmmap/flush all the fragment buffers used by the dropped > packet. 'alloc_failures' counter is incremented to mark the skb allocation > error in driver statistics. You're doing multiple things here, please split this patch up.
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c index 4746a6b258f0..2d9d95071786 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c @@ -336,6 +336,51 @@ static int octep_oq_check_hw_for_pkts(struct octep_device *oct, return new_pkts; } +/** + * octep_oq_next_pkt() - Move to the next packet in Rx queue. + * + * @oq: Octeon Rx queue data structure. + * @buff_info: Current packet buffer info. + * @read_idx: Current packet index in the ring. + * @desc_used: Current packet descriptor number. + * + * Free the resources associated with a packet. + * Increment packet index in the ring and packet descriptor number. + */ +static void octep_oq_next_pkt(struct octep_oq *oq, + struct octep_rx_buffer *buff_info, + u32 *read_idx, u32 *desc_used) +{ + dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr, + PAGE_SIZE, DMA_FROM_DEVICE); + buff_info->page = NULL; + (*read_idx)++; + (*desc_used)++; + if (*read_idx == oq->max_count) + *read_idx = 0; +} + +/** + * octep_oq_drop_rx() - Free the resources associated with a packet. + * + * @oq: Octeon Rx queue data structure. + * @buff_info: Current packet buffer info. + * @read_idx: Current packet index in the ring. + * @desc_used: Current packet descriptor number. + * + */ +static void octep_oq_drop_rx(struct octep_oq *oq, + struct octep_rx_buffer *buff_info, + u32 *read_idx, u32 *desc_used) +{ + int data_len = buff_info->len - oq->max_single_buffer_size; + + do { + octep_oq_next_pkt(oq, buff_info, read_idx, desc_used); + data_len -= oq->buffer_size; + } while (data_len > 0); +} + /** * __octep_oq_process_rx() - Process hardware Rx queue and push to stack. * @@ -367,10 +412,7 @@ static int __octep_oq_process_rx(struct octep_device *oct, desc_used = 0; for (pkt = 0; pkt < pkts_to_process; pkt++) { buff_info = (struct octep_rx_buffer *)&oq->buff_info[read_idx]; - dma_unmap_page(oq->dev, oq->desc_ring[read_idx].buffer_ptr, - PAGE_SIZE, DMA_FROM_DEVICE); resp_hw = page_address(buff_info->page); - buff_info->page = NULL; /* Swap the length field that is in Big-Endian to CPU */ buff_info->len = be64_to_cpu(resp_hw->length); @@ -394,36 +436,33 @@ static int __octep_oq_process_rx(struct octep_device *oct, data_offset = OCTEP_OQ_RESP_HW_SIZE; rx_ol_flags = 0; } + + skb = build_skb((void *)resp_hw, PAGE_SIZE); + if (!skb) { + octep_oq_drop_rx(oq, buff_info, + &read_idx, &desc_used); + oq->stats.alloc_failures++; + continue; + } + skb_reserve(skb, data_offset); + + octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used); + rx_bytes += buff_info->len; if (buff_info->len <= oq->max_single_buffer_size) { - skb = build_skb((void *)resp_hw, PAGE_SIZE); - skb_reserve(skb, data_offset); skb_put(skb, buff_info->len); - read_idx++; - desc_used++; - if (read_idx == oq->max_count) - read_idx = 0; } else { struct skb_shared_info *shinfo; u16 data_len; - skb = build_skb((void *)resp_hw, PAGE_SIZE); - skb_reserve(skb, data_offset); /* Head fragment includes response header(s); * subsequent fragments contains only data. */ skb_put(skb, oq->max_single_buffer_size); - read_idx++; - desc_used++; - if (read_idx == oq->max_count) - read_idx = 0; - shinfo = skb_shinfo(skb); 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_rx_buffer *) &oq->buff_info[read_idx]; if (data_len < oq->buffer_size) { @@ -438,11 +477,8 @@ static int __octep_oq_process_rx(struct octep_device *oct, buff_info->page, 0, buff_info->len, buff_info->len); - buff_info->page = NULL; - read_idx++; - desc_used++; - if (read_idx == oq->max_count) - read_idx = 0; + + octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used); } }
build_skb() returns NULL in case of a memory allocation failure so handle it inside __octep_oq_process_rx() to avoid NULL pointer dereference. __octep_oq_process_rx() is called during NAPI polling by the driver. If skb allocation fails, keep on pulling packets out of the Rx DMA queue: we shouldn't break the polling immediately and thus falsely indicate to the octep_napi_poll() that the Rx pressure is going down. As there is no associated skb in this case, don't process the packets and don't push them up the network stack - they are skipped. The common code with skb and some index manipulations is extracted to make the fix more readable and avoid code duplication. Also helper function is implemented to unmmap/flush all the fragment buffers used by the dropped packet. 'alloc_failures' counter is incremented to mark the skb allocation error in driver statistics. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 37d79d059606 ("octeon_ep: add Tx/Rx processing and interrupt support") Suggested-by: Paolo Abeni <pabeni@redhat.com> Suggested-by: Simon Horman <horms@kernel.org> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- A similar situation is present in the __octep_vf_oq_process_rx() of the Octeon VF driver. First we want to try the fix on __octep_oq_process_rx(). Compile tested only. v3: - Implement helper which frees current packet resources and increase index and descriptor as suggested by Simon (https://lore.kernel.org/all/20240919134812.GB1571683@kernel.org/) - Optimize helper as suggested by Paolo (https://lore.kernel.org/all/b9ae8575-f903-425f-aa42-0c2a7605aa94@redhat.com/) v2: https://lore.kernel.org/all/20240916060212.12393-1-amishin@t-argos.ru/ - Implement helper instead of adding multiple checks for '!skb' and remove 'rx_bytes' increasing in case of packet dropping as suggested by Paolo (https://lore.kernel.org/all/ba514498-3706-413b-a09f-f577861eef28@redhat.com/) v1: https://lore.kernel.org/all/20240906063907.9591-1-amishin@t-argos.ru/ .../net/ethernet/marvell/octeon_ep/octep_rx.c | 82 +++++++++++++------ 1 file changed, 59 insertions(+), 23 deletions(-)