Re: [PATCH v2 10/14] iommu/amd: Remove conditions from domain free paths

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Sep 02, 2024 at 05:33:50PM +0530, Vasant Hegde wrote:
> >  void protection_domain_free(struct protection_domain *domain)
> >  {
> > -	if (!domain)
> > -		return;
> > -
> >  	WARN_ON(!list_empty(&domain->dev_list));
> > -
> > -	if (domain->iop.pgtbl.cfg.tlb)
> > -		free_io_pgtable_ops(&domain->iop.pgtbl.ops);
> > -
> > -	if (domain->id)
> > -		domain_id_free(domain->id);
> > -
> > +	free_io_pgtable_ops(&domain->iop.pgtbl.ops);
> 
> This is common path gets for all domain type including passthrough, SVA.
> Hence its not yet ready for unconditional call.

Oh, I see, and ops isn't a pointer so it doesn't become NULL like
other drivers..

> This is causing NULL pointer dereference when I try to change domain
> type.

Yes

> For now, can you keep abvoe check -OR- add PAGING_DOMAIN check? I will revisit
> after implementing global static identity domain and fixing SVA code.

tlb is always NULL now that is why I had to do this.

@@ -2259,7 +2259,8 @@ static void cleanup_domain(struct protection_domain *domain)
 void protection_domain_free(struct protection_domain *domain)
 {
        WARN_ON(!list_empty(&domain->dev_list));
-       free_io_pgtable_ops(&domain->iop.pgtbl.ops);
+       if (domain->domain.type & __IOMMU_DOMAIN_PAGING)
+               free_io_pgtable_ops(&domain->iop.pgtbl.ops);
        domain_id_free(domain->id);
        kfree(domain);
 }

Is that OK?

Joerg can you squish it?

Jason