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

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 <[email protected]>

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: [email protected] # v5.2+

> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/n=
et/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)
> =20
>  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 polli=
ng
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=20
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 >=3D self->size) {
                    err =3D -EIO;
                    goto err_exit;
                }
    ...
                if (unlikely(!is_rsc_completed) ||
                    frag_cnt > MAX_SKB_FRAGS) {
                    err =3D 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 >=3D rx_ring->size) {
                    err =3D -EIO;
                    goto err_exit;
                }
                next_ =3D buff_->next;
                buff_ =3D &rx_ring->buff_ring[next_];
                is_rsc_completed =3D
                    aq_ring_dx_in_range(rx_ring->sw_head,
                                next_,
                                rx_ring->hw_head);

                if (unlikely(!is_rsc_completed))
                    break;

                buff->is_error |=3D buff_->is_error;
                buff->is_cso_err |=3D 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 sto=
rm
as in __aq_ring_rx_clean()?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/tencent_29B86031792=
[email protected]?part=3D2