Re: [PATCH v5 08/27] iommu/arm-smmu-v3: Move allocation of the cdtable into arm_smmu_get_cd_ptr()

[Mostafa Saleh <smostafa@google.com>]

On Mon, Mar 25, 2024 at 11:21:28AM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 22, 2024 at 07:07:10PM +0000, Mostafa Saleh wrote:
> > Hi Jason,
> > 
> > On Mon, Mar 04, 2024 at 07:43:56PM -0400, Jason Gunthorpe wrote:
> > > No reason to force callers to do two steps. Make arm_smmu_get_cd_ptr()
> > > able to return an entry in all cases except OOM
> >
> > I believe the current code is more clear, as it is explicit about which path
> > is expected to allocate.
> 
> I think we had this allocate vs no allocate discussion before on
> something else..
> 
> It would be good to make *full* allocate/noallocate variants of
> get_cd_ptr() and the cases that must never allocate call the no
> allocate variation. There are some issues with GFP_KERNEL/ATOMIC that
> are a bit hard to understand as well.
> 
> This is a bigger issue than just the cd_table, as even the leafs
> should not allocate.
> 
> > As there are many callers for arm_smmu_get_cd_ptr() directly and indirectly,
> > and it read-modify-writes the cdtable, it would be a pain to debug not
> > knowing which one could allocate, and this patch only abstracts one
> > allocating call, so it is not much code less.
> 
> > For example, (again I don’t know much about SVA) I think there might be a
> > race condition as follows:
> > arm_smmu_attach_dev
> > 	arm_smmu_domain_finalise() => set domain stage
> > 	[....]
> > 	arm_smmu_get_cd_ptr() => RMW master->cd_table
> > 
> > arm_smmu_sva_set_dev_pasid
> > 	__arm_smmu_sva_bind
> > 		Check stage is valid
> > 		[...]
> > 		arm_smmu_write_ctx_desc
> > 			arm_smmu_get_cd_ptr => RMW master->cd_table
> > 
> > If this path is true though, I guess the in current code, we would need some
> > barriers in arm_smmu_get_cd_ptr(), arm_smmu_get_cd_ptr()
> 
> Both of those functions are called under the group mutex held by the
> core code.
> 
> The only valid condition to change the CD table backing memory is when
> the group mutex is held. We now have iommu_group_mutex_assert() so an
> alloc/noalloc split can call that test on the alloc side which is
> motivation enough to do it, IMHO.
> 
> I will split the function and sort it all out, but I will still
> integrate the cd_table allocation into the allocating version of
> get_cd_ptr().

I prefer that, as it makes it clear which paths expect to allocate
and which not.

> Thanks,
> Jason

Thanks,
Mostafa