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 12:03:43 -0300	[thread overview]
Message-ID: <20231006150343.GS682044@nvidia.com> (raw)
In-Reply-To: <161e4152-9213-415f-8315-4f3a73b76022@arm.com>

On Fri, Oct 06, 2023 at 03:56:15PM +0100, Robin Murphy wrote:
> On 2023-10-06 14:53, Jason Gunthorpe wrote:
> > 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.
> 
> How? It should already be trivial to confirm that the driver is not itself
> making any reference to arm_smmu_identity_domain or arm_smmu_blocked_domain
> other than exposing them to the core API, with no more than a simple grep.
> And if the core code does somehow screw up at runtime and they get passed
> back into arm_smmu_attach_dev() then that's still going to use
> to_smmu_domain() on them and propagate that to its callees, with exactly the
> same effect as if they did so themselves. I fail to understand what you
> think you can achieve here.

I had to audit all of this to make sure. The to_smmu_domain() calls
here required some extra work to check because they are in an
interrupt, not a domain op.

I went through and checked it, why shouldn't I more fully document
that I checked it and it is in fact OK?

Why are you objecting to this? It clearly makes the thing easier to
follow to use the more specific types?!

> It's never worth doing anything for the sole reason of trying to make it
> slightly harder for someone who doesn't understand the code to break the
> code.

Maintainabiliy is making the code easier to understand as a merit on
its own :(

Jason

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