[PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Christophe JAILLET]

The only iommu function call in this driver is a
tegra_dev_iommu_get_stream_id() which does not allocate anything and does
not take any reference.

More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in
the probe.

So there is no point in calling iommu_fwspec_free() in the remove function.

Remove this incorrect function call.

Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only

This patch is completely speculative. *Review with care*.
---
 drivers/crypto/tegra/tegra-se-main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/tegra/tegra-se-main.c b/drivers/crypto/tegra/tegra-se-main.c
index 9955874b3dc3..f94c0331b148 100644
--- a/drivers/crypto/tegra/tegra-se-main.c
+++ b/drivers/crypto/tegra/tegra-se-main.c
@@ -326,7 +326,6 @@ static void tegra_se_remove(struct platform_device *pdev)
 
 	crypto_engine_stop(se->engine);
 	crypto_engine_exit(se->engine);
-	iommu_fwspec_free(se->dev);
 	host1x_client_unregister(&se->client);
 }
 
-- 
2.45.1

RE: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Akhil R]

> 
> The only iommu function call in this driver is a
> tegra_dev_iommu_get_stream_id() which does not allocate anything and does
> not take any reference.
> 
> More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
> probe.
> 
> So there is no point in calling iommu_fwspec_free() in the remove function.
> 
> Remove this incorrect function call.
> 
> Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only
> 
> This patch is completely speculative. *Review with care*.
> ---
>  drivers/crypto/tegra/tegra-se-main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/crypto/tegra/tegra-se-main.c b/drivers/crypto/tegra/tegra-se-
> main.c
> index 9955874b3dc3..f94c0331b148 100644
> --- a/drivers/crypto/tegra/tegra-se-main.c
> +++ b/drivers/crypto/tegra/tegra-se-main.c
> @@ -326,7 +326,6 @@ static void tegra_se_remove(struct platform_device
> *pdev)
> 
>         crypto_engine_stop(se->engine);
>         crypto_engine_exit(se->engine);
> -       iommu_fwspec_free(se->dev);
>         host1x_client_unregister(&se->client);
>  }
> 

This was a futile attempt to fix a kmemleak warning in host1x_client_unregister() in a very old kernel.
I don't see it anymore, either with or without this change. So,

Tested-by: Akhil R <akhilrajeev@nvidia.com>
Acked-by: Akhil R <akhilrajeev@nvidia.com>

Regards,
Akhil

RE: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Akhil R]

> The only iommu function call in this driver is a
> tegra_dev_iommu_get_stream_id() which does not allocate anything and does
> not take any reference.
> 
> More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
> probe.

I did not completely understand what is being tried to convey here.
If I understand it right, iommu_fwspec_free() does not do anything
with the "devm_kzalloc"ed variable. 

It would probably be a good idea to remove this line from the commit message.

> 
> So there is no point in calling iommu_fwspec_free() in the remove function.
> 
> Remove this incorrect function call.
> 
> Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Wed May 29, 2024 at 8:53 AM CEST, Akhil R wrote:
> > The only iommu function call in this driver is a
> > tegra_dev_iommu_get_stream_id() which does not allocate anything and does
> > not take any reference.
> > 
> > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
> > probe.
>
> I did not completely understand what is being tried to convey here.
> If I understand it right, iommu_fwspec_free() does not do anything
> with the "devm_kzalloc"ed variable. 
>
> It would probably be a good idea to remove this line from the commit message.

Yeah, I think that's a bit misleading. What iommu_fwspec_free() does is
get the iommu_fwspec from the passed-in device and then frees that
iommu_fwspec.

That said, as I was looking around I didn't spot anything that was
calling iommu_fwspec_free() in any of the cleanup paths, so either I'm
missing something or it's a real memory leak (though perhaps one that we
are ignoring on purpose because these are usually attached to devices
that don't just go away).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Herbert Xu]

On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote:
> > The only iommu function call in this driver is a
> > tegra_dev_iommu_get_stream_id() which does not allocate anything and does
> > not take any reference.
> > 
> > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
> > probe.
> 
> I did not completely understand what is being tried to convey here.
> If I understand it right, iommu_fwspec_free() does not do anything
> with the "devm_kzalloc"ed variable. 
> 
> It would probably be a good idea to remove this line from the commit message.

I think he means that as the memory was allocated via devm, it will
be automatically freed by the kernel and the driver does not need
to (and should not) free the memory by hand.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Akhil R]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Friday, May 31, 2024 10:53 AM
> To: Akhil R <akhilrajeev@nvidia.com>
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; David S. Miller
> <davem@davemloft.net>; Thierry Reding <thierry.reding@gmail.com>; Jon
> Hunter <jonathanh@nvidia.com>; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
> tegra@vger.kernel.org
> Subject: Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free()
> call in tegra_se_remove()
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote:
> > > The only iommu function call in this driver is a
> > > tegra_dev_iommu_get_stream_id() which does not allocate anything and
> does
> > > not take any reference.
> > >
> > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
> > > probe.
> >
> > I did not completely understand what is being tried to convey here.
> > If I understand it right, iommu_fwspec_free() does not do anything
> > with the "devm_kzalloc"ed variable.
> >
> > It would probably be a good idea to remove this line from the commit message.
> 
> I think he means that as the memory was allocated via devm, it will
> be automatically freed by the kernel and the driver does not need
> to (and should not) free the memory by hand.

Ya. But iommu_fwspec_free() does not free the memory allocated via devm.

I think iommu_fwspec_free() is expected to be called in symmetry with
iommu_fwspec_init(). So, I do agree that the SE driver does not allocate
what is freed by iommu_fwspec_free(), but I feel this line is a bit misleading.

Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Herbert Xu]

On Sat, May 25, 2024 at 05:14:35PM +0200, Christophe JAILLET wrote:
> The only iommu function call in this driver is a
> tegra_dev_iommu_get_stream_id() which does not allocate anything and does
> not take any reference.
> 
> More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in
> the probe.
> 
> So there is no point in calling iommu_fwspec_free() in the remove function.
> 
> Remove this incorrect function call.
> 
> Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only
> 
> This patch is completely speculative. *Review with care*.
> ---
>  drivers/crypto/tegra/tegra-se-main.c | 1 -
>  1 file changed, 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove()

[Marion & Christophe JAILLET]

Le 31/05/2024 à 07:36, Akhil R a écrit :
>> -----Original Message-----
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Sent: Friday, May 31, 2024 10:53 AM
>> To: Akhil R <akhilrajeev@nvidia.com>
>> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; David S. Miller
>> <davem@davemloft.net>; Thierry Reding <thierry.reding@gmail.com>; Jon
>> Hunter <jonathanh@nvidia.com>; linux-kernel@vger.kernel.org; kernel-
>> janitors@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
>> tegra@vger.kernel.org
>> Subject: Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free()
>> call in tegra_se_remove()
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote:
>>>> The only iommu function call in this driver is a
>>>> tegra_dev_iommu_get_stream_id() which does not allocate anything and
>> does
>>>> not take any reference.
>>>>
>>>> More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the
>>>> probe.
>>> I did not completely understand what is being tried to convey here.
>>> If I understand it right, iommu_fwspec_free() does not do anything
>>> with the "devm_kzalloc"ed variable.
>>>
>>> It would probably be a good idea to remove this line from the commit message.
>> I think he means that as the memory was allocated via devm, it will
>> be automatically freed by the kernel and the driver does not need
>> to (and should not) free the memory by hand.


Yes, that is my point.

> Ya. But iommu_fwspec_free() does not free the memory allocated via devm.
>
> I think iommu_fwspec_free() is expected to be called in symmetry with
> iommu_fwspec_init(). So, I do agree that the SE driver does not allocate
> what is freed by iommu_fwspec_free(), but I feel this line is a bit misleading.
>
Yes, I spoke too fast.
What is freed is dev_iommu_fwspec_get(dev);, not dev. So the sentence I 
wrote makes no sense and should be removed :(


CJ