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

[Jason Gunthorpe <jgg@nvidia.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