Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Dec 2022 10:55:30 +0100
From:   Lorenzo Bianconi <lorenzo@kernel.org>
To:     Bo Jiao <bo.jiao@mediatek.com>
Cc:     Felix Fietkau <nbd@nbd.name>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Sujuan Chen <sujuan.chen@mediatek.com>,
        Shayne Chen <shayne.chen@mediatek.com>,
        Evelyn Tsai <evelyn.tsai@mediatek.com>,
        linux-mediatek <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v2] wifi: mt76: fix potential memory leakage
Message-ID: <Y6A1Et1yzcWnWhpy@lore-desk>
References: <0a218783098a6b1b90ff120c7182acee93dc8feb.1671423784.git.Bo.Jiao@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="GYX6T5wvBRqxBDpz"
Content-Disposition: inline
In-Reply-To: <0a218783098a6b1b90ff120c7182acee93dc8feb.1671423784.git.Bo.Jiao@mediatek.com>
Precedence: bulk


--GYX6T5wvBRqxBDpz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

> From: Bo Jiao <Bo.Jiao@mediatek.com>
>=20
> fix potential memory leakage, recycle rxwi when mt76_dma_add_buf() call f=
ail.
>=20
> Signed-off-by: Bo Jiao <Bo.Jiao@mediatek.com>
> ---
> v2:
> - recycle rxwi when page_frag_alloc() and dma_map_single() fail.
> ---
>  drivers/net/wireless/mediatek/mt76/dma.c | 27 ++++++++++++++----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>=20
> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wirel=
ess/mediatek/mt76/dma.c
> index fc24b35..76ad47d 100644
> --- a/drivers/net/wireless/mediatek/mt76/dma.c
> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
> @@ -580,24 +580,29 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct mt76_=
queue *q)
> =20
>  		buf =3D page_frag_alloc(&q->rx_page, q->buf_size, GFP_ATOMIC);
>  		if (!buf)
> -			break;
> +			goto out;
> =20
>  		addr =3D dma_map_single(dev->dma_dev, buf, len, DMA_FROM_DEVICE);
> -		if (unlikely(dma_mapping_error(dev->dma_dev, addr))) {
> -			skb_free_frag(buf);
> -			break;
> -		}
> +		if (unlikely(dma_mapping_error(dev->dma_dev, addr)))
> +			goto free;
> =20
>  		qbuf.addr =3D addr + offset;
>  		qbuf.len =3D len - offset;
>  		qbuf.skip_unmap =3D false;
> -		if (mt76_dma_add_buf(dev, q, &qbuf, 1, 0, buf, t) < 0) {
> -			dma_unmap_single(dev->dma_dev, addr, len,
> -					 DMA_FROM_DEVICE);
> -			skb_free_frag(buf);
> -			break;
> -		}
> +		if (mt76_dma_add_buf(dev, q, &qbuf, 1, 0, buf, t) < 0)
> +			goto umap;
> +
>  		frames++;
> +		continue;
> +
> +umap:
> +		dma_unmap_single(dev->dma_dev, addr, len,
> +				 DMA_FROM_DEVICE);
> +free:
> +		skb_free_frag(buf);
> +out:
> +		mt76_put_rxwi(dev, t);
> +		break;
>  	}
> =20
>  	if (frames)

Hi Bo,

I guess in the way below, the code is more readable, what do you think?

Regards,
Lorenzo

diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireles=
s/mediatek/mt76/dma.c
index fad5fe19fe18..001538f698f1 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -571,13 +571,6 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct mt76_que=
ue *q)
 		struct mt76_queue_buf qbuf;
 		void *buf =3D NULL;
=20
-		if ((q->flags & MT_QFLAG_WED) &&
-		    FIELD_GET(MT_QFLAG_WED_TYPE, q->flags) =3D=3D MT76_WED_Q_RX) {
-			t =3D mt76_get_rxwi(dev);
-			if (!t)
-				break;
-		}
-
 		buf =3D page_frag_alloc(&q->rx_page, q->buf_size, GFP_ATOMIC);
 		if (!buf)
 			break;
@@ -588,16 +581,27 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct mt76_qu=
eue *q)
 			break;
 		}
=20
+		if ((q->flags & MT_QFLAG_WED) &&
+		    FIELD_GET(MT_QFLAG_WED_TYPE, q->flags) =3D=3D MT76_WED_Q_RX) {
+			t =3D mt76_get_rxwi(dev);
+			if (!t)
+				goto unmap;
+		}
+
 		qbuf.addr =3D addr + offset;
 		qbuf.len =3D len - offset;
 		qbuf.skip_unmap =3D false;
-		if (mt76_dma_add_buf(dev, q, &qbuf, 1, 0, buf, t) < 0) {
-			dma_unmap_single(dev->dma_dev, addr, len,
-					 DMA_FROM_DEVICE);
-			skb_free_frag(buf);
-			break;
+
+		if (!mt76_dma_add_buf(dev, q, &qbuf, 1, 0, buf, t)) {
+			frames++;
+			continue;
 		}
-		frames++;
+
+unmap:
+		dma_unmap_single(dev->dma_dev, addr, len, DMA_FROM_DEVICE);
+		skb_free_frag(buf);
+		mt76_put_rxwi(dev, t);
+		break;
 	}
=20
 	if (frames)

> --=20
> 2.18.0
>=20

--GYX6T5wvBRqxBDpz
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCY6A1EgAKCRA6cBh0uS2t
rBwPAQD44i+a+7jteXHTxdG433JNJtOhfHYBcIa/JOOLWl61SQD+J+RUiW2LG8BU
LnZeQn1Ykxqe0g+zW03IxQfjXMv7qw0=
=27vm
-----END PGP SIGNATURE-----

--GYX6T5wvBRqxBDpz--