Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Will Deacon <will@kernel.org>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Heiko Stuebner <heiko@sntech.de>, Joerg Roedel <jroedel@suse.de>,
	Jerry Snitselaar <jsnitsel@redhat.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Steven Price <steven.price@arm.com>
Subject: Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
Date: Fri, 6 Oct 2023 10:53:47 -0300	[thread overview]
Message-ID: <20231006135347.GR682044@nvidia.com> (raw)
In-Reply-To: <db259336-1203-4d12-ac80-3108aaf5ec82@arm.com>

On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
>
> Is there something inherently difficult about to_smmu_domain()? It's hard to
> tell how the aforementioned checks might expect to work since they don't
> appear to be added anywhere :/

?? There are not added checks, this is talking about static checks and
code auditing.

Let's try the commit paragraph again:

Now that we have IDENTITY and BLOCKED domains that do not use the
struct arm_smmu_domain it is important that to_smmu_domain() is only
called on iommu_domain structs passed to the paging domain ops (aka
default_domain_ops). Use the more specific type in several call
chains and remove the few to_smmu_domain() calls that are not
obviously in an op call chain.

This makes it easier to audit the code that the two IDENTITY and
BLOCKED domains are not leaking someplace they should not.

> > @@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >   	struct io_pgtable_ops *pgtbl_ops;
> >   	struct io_pgtable_cfg pgtbl_cfg;
> >   	enum io_pgtable_fmt fmt;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> 
> If at all, I think I'd rather just flip this to a local "struct iommu_domain
> *domain = &smmu_domain->domain;" and avoid the hunk of churn below.

Sure

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel