Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\))
Subject: Re: [PATCH] function mana_hwc_create_wq leaks memory
From:   Dhiraj Shah <find.dhiraj@gmail.com>
In-Reply-To: <BYAPR21MB127087B408352336E0A8BF2ABF359@BYAPR21MB1270.namprd21.prod.outlook.com>
Date:   Thu, 10 Jun 2021 20:53:10 +0100
Cc:     KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Shachar Raindel <shacharr@microsoft.com>,
        Colin Ian King <colin.king@canonical.com>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <98DC11D9-668E-47F2-891C-6F41E70BD5F4@gmail.com>
References: <20210610152925.18145-1-find.dhiraj@gmail.com>
 <BYAPR21MB1270FC995760BE925179F353BF359@BYAPR21MB1270.namprd21.prod.outlook.com>
 <BYAPR21MB127087B408352336E0A8BF2ABF359@BYAPR21MB1270.namprd21.prod.outlook.com>
To:     Dexuan Cui <decui@microsoft.com>
Precedence: bulk

Hi Dexuan,

Thanks for the feedback.

You are right saying =E2=80=98mana_hwc_destroy_wq' free=E2=80=99s up the =
queue.=20

However for example if  function 'mana_hwc_alloc_dma_buf' fails it will =
goto =E2=80=98out' and  call  =E2=80=98mana_hwc_destroy_wq', the value =
'hwc_wq->gdma_wq' is still not assigned at this point. In the  =
=E2=80=98mana_hwc_destroy_wq' function i see it checks for =
'hwc_wq->gdma_wq' before calling, =E2=80=98mana_gd_destroy_queue', which =
makes me think queue is still not freed.

Please let me know if i am missing something.

Regards,
/Dhiraj

> On 10 Jun 2021, at 18:28, Dexuan Cui <decui@microsoft.com> wrote:
>=20
>> From: Dexuan Cui <decui@microsoft.com>
>> Sent: Thursday, June 10, 2021 10:18 AM
>> ...
>>> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
>>> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>>> index 1a923fd99990..4aa4bda518fb 100644
>>> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
>>> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
>>> @@ -501,8 +501,10 @@ static int mana_hwc_create_wq(struct
>>> hw_channel_context *hwc,
>>> 	*hwc_wq_ptr =3D hwc_wq;
>>> 	return 0;
>>> out:
>>> -	if (err)
>>> +	if (err) {
>>=20
>> Here the 'err' must be non-zero. Can you please remove this 'if'?
>>=20
>>> +		kfree(queue);
>>> 		mana_hwc_destroy_wq(hwc, hwc_wq);
>>> +	}
>>> 	return err;
>>> }
>>=20
>> Reviewed-by: Dexuan Cui <decui@microsoft.com>
>=20
> Hi Dhiraj,
> I checked the code again and it looks like your patch is actually
> unnecessary as IMO there is no memory leak here: the 'queue'
> pointer is passed to mana_hwc_destroy_wq() as hwc_wq->gdma_wq,
> and is later freed in mana_gd_destroy_queue() ->
> mana_gd_destroy_queue().
>=20
> The 'if' test can be removed as the 'err's is always non-zero there.
>=20
> Thanks,
> Dexuan