Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <bf79924a-2501-c835-6bf8-ab2810df8e92@linux.intel.com>
Date:   Thu, 12 May 2022 20:39:16 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Cc:     baolu.lu@linux.intel.com, Joerg Roedel <joro@8bytes.org>,
        Christoph Hellwig <hch@infradead.org>,
        Kevin Tian <kevin.tian@intel.com>,
        Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Jean-Philippe Brucker <jean-philippe@linaro.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Vinod Koul <vkoul@kernel.org>,
        Eric Auger <eric.auger@redhat.com>,
        Liu Yi L <yi.l.liu@intel.com>,
        Jacob jun Pan <jacob.jun.pan@intel.com>,
        iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 08/12] iommu/sva: Use attach/detach_pasid_dev in SVA
 interfaces
Content-Language: en-US
To:     Jason Gunthorpe <jgg@nvidia.com>
References: <20220510061738.2761430-1-baolu.lu@linux.intel.com>
 <20220510061738.2761430-9-baolu.lu@linux.intel.com>
 <20220510152330.GG49344@nvidia.com>
 <749a7d62-3e6c-ef5c-beaf-6b7add495740@linux.intel.com>
 <20220511145319.GZ49344@nvidia.com>
 <05a68e1e-8e18-5914-ebe7-d7b1a4aaa2ec@linux.intel.com>
 <20220512115136.GV49344@nvidia.com>
From:   Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <20220512115136.GV49344@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/5/12 19:51, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote:
>>>> +       mutex_lock(&group->mutex);
>>>> +       domain = xa_load(&group->pasid_array, pasid);
>>>> +       if (domain && domain->type != type)
>>>> +               domain = NULL;
>>>> +       mutex_unlock(&group->mutex);
>>>> +       iommu_group_put(group);
>>>> +
>>>> +       return domain;
>>> This is bad locking, group->pasid_array values cannot be taken outside
>>> the lock.
>>
>> It's not iommu core, but SVA (or other feature components) that manage
>> the life cycle of a domain. The iommu core only provides a place to
>> store the domain pointer. The feature components are free to fetch their
>> domain pointers from iommu core as long as they are sure that the domain
>> is alive during use.
> 
> I'm not convinced.

I'm sorry, I may not have explained it clearly. :-)

This helper is safe for uses inside the IOMMU subsystem. We could trust
ourselves that nobody will abuse this helper to obtain domains belonging
to others as the pasid has been allocated for SVA code. No other code
should be able to setup another type of domain for this pasid. The SVA
code has its own lock mechanism to avoid using after free.

Please correct me if I missed anything. :-) By the way, I can see some
similar helpers in current IOMMU core. For example,

struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
{
         struct iommu_domain *domain;
         struct iommu_group *group;

         group = iommu_group_get(dev);
         if (!group)
                 return NULL;

         domain = group->domain;

         iommu_group_put(group);

         return domain;
}
EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);

Best regards,
baolu