Re: [PATCH net 2/2] net: atlantic: free RX pages of consumed but not refilled buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Malformed or oversized RX descriptor chains cause a permanent NAPI polling lockup or hard infinite loop.
--

--- Patch [2]: [PATCH net 2/2] net: atlantic: free RX pages of consumed but not refilled buffers ---
commit f13d9fd8fe93327bbe27469851140df3b966b786
Author: Yangyu Chen <cyy@cyyself.name>

net: atlantic: free RX pages of consumed but not refilled buffers

aq_ring_rx_deinit() only walks the region posted to hardware. Slots that
were consumed but not yet reposted sit in a gap with a live page, and the
deinit walk never visits them, causing a page and DMA mapping leak. This
patch walks the whole ring instead and releases whatever is still there.

Cc: stable@vger.kernel.org # v5.2+

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> index 81685a4dc5a6d..e1193c6719d9d 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c

[ ... ]

> @@ -950,15 +950,29 @@ int aq_ring_rx_fill(struct aq_ring_s *self)
>  
>  void aq_ring_rx_deinit(struct aq_ring_s *self)
>  {

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the error handling in
__aq_ring_rx_clean() and __aq_ring_xdp_clean() cause a permanent NAPI polling
lockup or an infinite loop when processing malformed RX descriptors?

In __aq_ring_rx_clean(), if a descriptor chain is out of bounds or exceeds the 
maximum fragment count, the function exits early without advancing sw_head:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:__aq_ring_rx_clean() {
    ...
                if (buff_->next >= self->size) {
                    err = -EIO;
                    goto err_exit;
                }
    ...
                if (unlikely(!is_rsc_completed) ||
                    frag_cnt > MAX_SKB_FRAGS) {
                    err = 0;
                    goto err_exit;
                }
    ...
}

Because sw_head is not advanced, wouldn't the next NAPI poll process the
exact same malformed descriptor again, resulting in an infinite NAPI polling
loop?

Additionally, in __aq_ring_xdp_clean(), there is a do-while loop that lacks a
fragment count limit:

drivers/net/ethernet/aquantia/atlantic/aq_ring.c:__aq_ring_xdp_clean() {
    ...
            do {
                if (buff_->next >= rx_ring->size) {
                    err = -EIO;
                    goto err_exit;
                }
                next_ = buff_->next;
                buff_ = &rx_ring->buff_ring[next_];
                is_rsc_completed =
                    aq_ring_dx_in_range(rx_ring->sw_head,
                                next_,
                                rx_ring->hw_head);

                if (unlikely(!is_rsc_completed))
                    break;

                buff->is_error |= buff_->is_error;
                buff->is_cso_err |= buff_->is_cso_err;
            } while (!buff_->is_eop);
    ...
}

If the descriptor chain is cyclic and within the valid range, could this
cause a hard infinite loop inside the softirq handler?

Also, if the index is out of bounds in __aq_ring_xdp_clean(), it jumps to
err_exit without advancing sw_head. Could this cause the same NAPI poll storm
as in __aq_ring_rx_clean()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_29B860317921D68DE77C718242DA418EB608@qq.com?part=2