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:   Wed, 21 Dec 2022 11:26:21 +0100
From:   "lorenzo@kernel.org" <lorenzo@kernel.org>
To:     Sujuan Chen =?utf-8?B?KOmZiOe0oOWonyk=?= 
        <Sujuan.Chen@mediatek.com>
Cc:     Bo Jiao =?utf-8?B?KOeEpuazoik=?= <Bo.Jiao@mediatek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        Shayne Chen =?utf-8?B?KOmZs+i7kuS4nik=?= 
        <Shayne.Chen@mediatek.com>, "nbd@nbd.name" <nbd@nbd.name>,
        Evelyn Tsai =?utf-8?B?KOiUoeePiumIuik=?= 
        <Evelyn.Tsai@mediatek.com>, Ryder Lee <Ryder.Lee@mediatek.com>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v2] wifi: mt76: fix potential memory leakage
Message-ID: <Y6LfTUoSZ8W/gJ0Y@lore-desk>
References: <0a218783098a6b1b90ff120c7182acee93dc8feb.1671423784.git.Bo.Jiao@mediatek.com>
 <Y6A1Et1yzcWnWhpy@lore-desk>
 <9d8aa92e3650128c94af91435a222c00ff2acb1c.camel@mediatek.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="SyFKwzvfz74vbiYO"
Content-Disposition: inline
In-Reply-To: <9d8aa92e3650128c94af91435a222c00ff2acb1c.camel@mediatek.com>
Precedence: bulk


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

[...]

> Hi Lore,

Hi Sujuan,

>=20
> we love your patch, but we have another patch to avoid memory
> fragmentation by duplicating the rx skb after mt76_dma_dequeue(). it
> requires mt76_get_rxwi() be placed before page_frag_alloc().
>=20
> the below patch(need rebase) will be sent after the current patch is
> accepted.
>=20
> --- a/drivers/net/wireless/mediatek/mt76/dma.c
> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
> @@ -386,9 +386,11 @@ mt76_dma_get_buf(struct mt76_dev *dev, struct
> mt76_queue *q, int idx,
>                                  SKB_WITH_OVERHEAD(q->buf_size),
>                                  DMA_FROM_DEVICE);
>=20
> -               buf =3D t->ptr;
> +               buf =3D page_frag_alloc(&q->rx_page, q->buf_size,
> GFP_ATOMIC);
> +               if (!buf)
> +                       return NULL;
> +               memcpy(buf, t->ptr, SKB_WITH_OVERHEAD(q->buf_size));

We this approach we still need to allocate the buffer (in mt76_dma_get_buf()
instead of mt76_dma_rx_fill()) but we need even to copy the full buffer that
can be pretty expensive instead of relying on the DMA, so I would avoid this
approach.

Regards,
Lorenzo

>                 t->dma_addr =3D 0;
> -               t->ptr =3D NULL;
>=20
>                 mt76_put_rxwi(dev, t);
>=20
> @@ -569,6 +571,7 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct
> mt76_queue *q)
>         while (q->queued < q->ndesc - 1) {
>                 struct mt76_txwi_cache *t =3D NULL;
>                 struct mt76_queue_buf qbuf;
> +               bool skip_alloc =3D false;
>                 void *buf =3D NULL;
>=20
>                 if ((q->flags & MT_QFLAG_WED) &&
> @@ -576,11 +579,19 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct
> mt76_queue *q)
>                         t =3D mt76_get_rxwi(dev);
>                         if (!t)
>                                 break;
> +
> +                       if (t->ptr) {
> +                               skip_alloc =3D true;
> +                               buf =3D t->ptr;
> +                       }
>                 }
>=20
> -               buf =3D page_frag_alloc(&q->rx_page, q->buf_size,
> GFP_ATOMIC);
> -               if (!buf)
> -                       break;
> +               if (!skip_alloc) {
> +                       buf =3D page_frag_alloc(&q->rx_page, q->buf_size,
> +                                             GFP_ATOMIC);
> +                       if (!buf)
> +                               break;
> +               }
>=20
>                 addr =3D dma_map_single(dev->dma_dev, buf, len,
> DMA_FROM_DEVICE);
>                 if (unlikely(dma_mapping_error(dev->dma_dev, addr))) {
> --
> 2.18.0
>=20
> > > --=20
> > > 2.18.0
> > >=20

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

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

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCY6LfTQAKCRA6cBh0uS2t
rBF/AP0ce3mIwZ/Uk5Zj1Y0rWsZg6hECTlHMNDdzISxhS4OuiwD/RSy8M9Sf3rVK
R4S5GCzFMqSqiU0ogdBVoRnk0nsccg0=
=wWbO
-----END PGP SIGNATURE-----

--SyFKwzvfz74vbiYO--