[bug report] iommu/vt-d: Add SVA domain support

[bug report] iommu/vt-d: Add SVA domain support

[Dan Carpenter]

Hello Lu Baolu,

The patch eaca8889a1ef: "iommu/vt-d: Add SVA domain support" from Oct
31, 2022, leads to the following Smatch static checker warning:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:213 adreno_iommu_create_address_space() warn: passing zero to 'ERR_CAST'
drivers/gpu/drm/msm/adreno/adreno_gpu.c:216 adreno_iommu_create_address_space() warn: 'geometry' isn't an ERR_PTR
drivers/iommu/intel/iommu.c:4721 intel_iommu_remove_dev_pasid() error: 'domain' dereferencing possible ERR_PTR()
drivers/iommu/io-pgfault.c:82 iopf_handler() error: 'domain' dereferencing possible ERR_PTR()

drivers/iommu/intel/iommu.c
    4713 static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
    4714 {
    4715         struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
    4716         struct iommu_domain *domain;
    4717 
    4718         /* Domain type specific cleanup: */
    4719         domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
    4720         if (domain) {

The iommu_get_domain_for_dev_pasid() function returns both error pointers
and NULL.  The comments say that it's supposed to only return NULL.  I
don't really understand what's going with these iommu functions.

--> 4721                 switch (domain->type) {
                                 ^^^^^^^^^^^^
    4722                 case IOMMU_DOMAIN_SVA:
    4723                         intel_svm_remove_dev_pasid(dev, pasid);
    4724                         break;
    4725                 default:
    4726                         /* should never reach here */
    4727                         WARN_ON(1);
    4728                         break;
    4729                 }
    4730         }
    4731 
    4732         intel_pasid_tear_down_entry(iommu, dev, pasid, false);
    4733 }

regards,
dan carpenter

Re: [bug report] iommu/vt-d: Add SVA domain support

[Baolu Lu]

On 11/15/22 9:04 PM, Dan Carpenter wrote:
> Hello Lu Baolu,
> 

Hi Dan,

> The patch eaca8889a1ef: "iommu/vt-d: Add SVA domain support" from Oct
> 31, 2022, leads to the following Smatch static checker warning:
> 
> drivers/gpu/drm/msm/adreno/adreno_gpu.c:213 adreno_iommu_create_address_space() warn: passing zero to 'ERR_CAST'
> drivers/gpu/drm/msm/adreno/adreno_gpu.c:216 adreno_iommu_create_address_space() warn: 'geometry' isn't an ERR_PTR
> drivers/iommu/intel/iommu.c:4721 intel_iommu_remove_dev_pasid() error: 'domain' dereferencing possible ERR_PTR()
> drivers/iommu/io-pgfault.c:82 iopf_handler() error: 'domain' dereferencing possible ERR_PTR()
> 
> drivers/iommu/intel/iommu.c
>      4713 static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>      4714 {
>      4715         struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
>      4716         struct iommu_domain *domain;
>      4717
>      4718         /* Domain type specific cleanup: */
>      4719         domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
>      4720         if (domain) {
> 
> The iommu_get_domain_for_dev_pasid() function returns both error pointers
> and NULL.  The comments say that it's supposed to only return NULL.  I
> don't really understand what's going with these iommu functions.

The iommu_get_domain_for_dev_pasid() function returns ERR_PTR(-EBUSY) if
the requested domain is not the same type as the one stored in the iommu
group.

But as the kernel-doc of iommu_get_domain_for_dev_pasid() states, type 0
means any match, hence, here @domain is either a valid pointer or NULL.

The same thing for iopf_handler().

Best regards,
baolu

> 
> --> 4721                 switch (domain->type) {
>                                   ^^^^^^^^^^^^
>      4722                 case IOMMU_DOMAIN_SVA:
>      4723                         intel_svm_remove_dev_pasid(dev, pasid);
>      4724                         break;
>      4725                 default:
>      4726                         /* should never reach here */
>      4727                         WARN_ON(1);
>      4728                         break;
>      4729                 }
>      4730         }
>      4731
>      4732         intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>      4733 }
> 
> regards,
> dan carpenter