linux-kernel - Re: [PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z/Qdewr7ZqwfK1ZV@nvidia.com>
Date: Mon, 7 Apr 2025 11:46:19 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <thierry.reding@...il.com>, <vdumpa@...dia.com>,
	<robin.murphy@....com>, <joro@...tes.org>, <jonathanh@...dia.com>,
	<linux-tegra@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<iommu@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to
 dmam_free_coherent()

On Mon, Apr 07, 2025 at 02:44:08PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 07, 2025 at 01:34:59AM -0700, Nicolin Chen wrote:
> > Two WARNINGs are observed when SMMU driver rolls back upon failure:
> >  arm-smmu-v3.9.auto: Failed to register iommu
> >  arm-smmu-v3.9.auto: probe with driver arm-smmu-v3 failed with error -22
> >  ------------[ cut here ]------------
> >  WARNING: CPU: 5 PID: 1 at kernel/dma/mapping.c:74 dmam_free_coherent+0xc0/0xd8
> >  Call trace:
> >   dmam_free_coherent+0xc0/0xd8 (P)
> >   tegra241_vintf_free_lvcmdq+0x74/0x188
> >   tegra241_cmdqv_remove_vintf+0x60/0x148
> >   tegra241_cmdqv_remove+0x48/0xc8
> >   arm_smmu_impl_remove+0x28/0x60
> >   devm_action_release+0x1c/0x40
> >  ------------[ cut here ]------------
> >  128 pages are still in use!
> >  WARNING: CPU: 16 PID: 1 at mm/page_alloc.c:6902 free_contig_range+0x18c/0x1c8
> >  Call trace:
> >   free_contig_range+0x18c/0x1c8 (P)
> >   cma_release+0x154/0x2f0
> >   dma_free_contiguous+0x38/0xa0
> >   dma_direct_free+0x10c/0x248
> >   dma_free_attrs+0x100/0x290
> >   dmam_free_coherent+0x78/0xd8
> >   tegra241_vintf_free_lvcmdq+0x74/0x160
> >   tegra241_cmdqv_remove+0x98/0x198
> >   arm_smmu_impl_remove+0x28/0x60
> >   devm_action_release+0x1c/0x40
> > 
> > For the first warning: when the main SMMU driver cleans up its resources,
> > any routine in arm_smmu_impl_remove() should not use any devres function.
> 
> Bleck. This is situations where you should not be using devres at all.
> 
> It is not that arm_smmu_impl_remove() should not use devres, the
> problem is that arm_smmu_impl_probe() has mis-ordered the devres
> callbacks if ops->device_remove() is going to be manually freeing
> things that probe allocated.
> 
> IMHO you should just put the goto unwind back into arm_smmu_device()
> probe and not use devm for ops->device_remove(). That will put things
> in their proper order and no problem.

I did that in my first attempt but it didn't keep the "fallback
to standard SMMU" part as the driver was..

But giving it a second thought, I think this fallback might not
be necessary at all since a structure allocation failure so the
standard SMMU driver will unlikely be able to continue normally.

I think the correct way is to fail init_structures and that will
ask SMMU driver to unwind with smmu->impl_ops->device_remove, as
you suggested here.

Thanks
Nicolin