[PATCH 0/4] iommu/vt-d: Several fixes for intel iommu driver

[PATCH 0/4] iommu/vt-d: Several fixes for intel iommu driver

[Tina Zhang]

This series includes several small fixes.

Tina Zhang (4):
  iommu/vt-d: Fix null pointer access in invalidate_range callback
  iommu/vt-d: Fix operand size in bitwise operation
  iommu/vt-d: Fix quoted string split across lines
  iommu/vt-d: Replace BUG()/BUG_ON() with WARN_ON/WARN_ON_ONCE()

 drivers/iommu/intel/dmar.c  |  8 ++++---
 drivers/iommu/intel/iommu.c | 46 ++++++++++++++++++++++---------------
 drivers/iommu/intel/svm.c   |  2 +-
 3 files changed, 34 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range callback

[Tina Zhang]

Add the missing check to avoid null pointer dereference.

The patch fixes below issue reported by klocwork tool:
Pointer 'info' returned from call to function 'dev_iommu_priv_get'
at line 180 may be NULL and may be dereferenced at line 186.

This patch can fix a potential null pointer dereference issue of
releasing a device working in svm mode.

Reported-by: Yongwei Ma <yongwei.ma@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/svm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 7367f56c3bad..837c1a4642e7 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -179,7 +179,7 @@ static void __flush_svm_range_dev(struct intel_svm *svm,
 {
 	struct device_domain_info *info = dev_iommu_priv_get(sdev->dev);
 
-	if (WARN_ON(!pages))
+	if (WARN_ON(!info || !pages))
 		return;
 
 	qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, pages, ih);
-- 
2.34.1

[PATCH 2/4] iommu/vt-d: Fix operand size in bitwise operation

[Tina Zhang]

The patch fixes the klocwork issues that operands in a bitwise operation have
different size at line 1692 of dmar.c, line 1898 and line 1907 of iommu.c.

Reported-by: Yongwei Ma <yongwei.ma@intel.com>
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/dmar.c  | 2 +-
 drivers/iommu/intel/iommu.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 6acfe879589c..7c77a59889ea 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -1689,7 +1689,7 @@ static void __dmar_enable_qi(struct intel_iommu *iommu)
 	 * is present.
 	 */
 	if (ecap_smts(iommu->ecap))
-		val |= (1 << 11) | 1;
+		val |= (1ull << 11) | 1ull;
 
 	raw_spin_lock_irqsave(&iommu->register_lock, flags);
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7c2f4bd33582..085d31a77f6f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1895,7 +1895,7 @@ context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
  */
 static inline void context_set_sm_dte(struct context_entry *context)
 {
-	context->lo |= (1 << 2);
+	context->lo |= (1ull << 2);
 }
 
 /*
@@ -1904,7 +1904,7 @@ static inline void context_set_sm_dte(struct context_entry *context)
  */
 static inline void context_set_sm_pre(struct context_entry *context)
 {
-	context->lo |= (1 << 4);
+	context->lo |= (1ull << 4);
 }
 
 /* Convert value to context PASID directory size field coding. */
-- 
2.34.1

[PATCH 3/4] iommu/vt-d: Fix quoted string split across lines

[Tina Zhang]

This patch resolves checkpatch warnings for intel iommu driver.

WARNING: quoted string split across lines
	pr_err("%s: iommu width (%d) is not "
		"sufficient for the mapped address (%llx)\n",

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/iommu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 085d31a77f6f..286bb17973a4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4294,8 +4294,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
 		/* check if minimum agaw is sufficient for mapped address */
 		end = __DOMAIN_MAX_ADDR(dmar_domain->gaw) + 1;
 		if (end < max_addr) {
-			pr_err("%s: iommu width (%d) is not "
-			       "sufficient for the mapped address (%llx)\n",
+			pr_err("%s: iommu width (%d) is not sufficient for the mapped address (%llx)\n",
 			       __func__, dmar_domain->gaw, max_addr);
 			return -EFAULT;
 		}
-- 
2.34.1

[PATCH 4/4] iommu/vt-d: Replace BUG()/BUG_ON() with WARN_ON/WARN_ON_ONCE()

[Tina Zhang]

Remove legacy BUG()/BUG_ON() and turn them into WARN_ON/WARN_ON_ONCE()
with errors.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/dmar.c  |  6 ++++--
 drivers/iommu/intel/iommu.c | 39 ++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 7c77a59889ea..39b0bad000ea 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -127,7 +127,8 @@ dmar_alloc_pci_notify_info(struct pci_dev *dev, unsigned long event)
 	struct pci_dev *tmp;
 	struct dmar_pci_notify_info *info;
 
-	BUG_ON(dev->is_virtfn);
+	if (WARN_ON(dev->is_virtfn))
+		return NULL;
 
 	/*
 	 * Ignore devices that have a domain number higher than what can
@@ -264,7 +265,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
 						   get_device(dev));
 				return 1;
 			}
-		BUG_ON(i >= devices_cnt);
+		if (WARN_ON_ONCE(i >= devices_cnt))
+			return -EINVAL;
 	}
 
 	return 0;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 286bb17973a4..20f7a9b3a6a7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -391,6 +391,14 @@ static inline int domain_pfn_supported(struct dmar_domain *domain,
 	return !(addr_width < BITS_PER_LONG && pfn >> addr_width);
 }
 
+static inline bool domain_pfn_range_valid(struct dmar_domain *domain,
+					  unsigned long start_pfn,
+					  unsigned long last_pfn)
+{
+	return (!domain_pfn_supported(domain, last_pfn)) ? false :
+		(start_pfn > last_pfn) ? false : true;
+}
+
 /*
  * Calculate the Supported Adjusted Guest Address Widths of an IOMMU.
  * Refer to 11.4.2 of the VT-d spec for the encoding of each bit of
@@ -915,7 +923,8 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
 	int level = agaw_to_level(domain->agaw);
 	int offset;
 
-	BUG_ON(!domain->pgd);
+	if (WARN_ON_ONCE(!domain->pgd))
+		return NULL;
 
 	if (!domain_pfn_supported(domain, pfn))
 		/* Address beyond IOMMU's addressing capabilities. */
@@ -1005,9 +1014,8 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
 	unsigned int large_page;
 	struct dma_pte *first_pte, *pte;
 
-	BUG_ON(!domain_pfn_supported(domain, start_pfn));
-	BUG_ON(!domain_pfn_supported(domain, last_pfn));
-	BUG_ON(start_pfn > last_pfn);
+	if (WARN_ON_ONCE(!domain_pfn_range_valid(domain, start_pfn, last_pfn))
+		return;
 
 	/* we don't need lock here; nobody else touches the iova range */
 	do {
@@ -1166,9 +1174,8 @@ static void dma_pte_clear_level(struct dmar_domain *domain, int level,
 static void domain_unmap(struct dmar_domain *domain, unsigned long start_pfn,
 			 unsigned long last_pfn, struct list_head *freelist)
 {
-	BUG_ON(!domain_pfn_supported(domain, start_pfn));
-	BUG_ON(!domain_pfn_supported(domain, last_pfn));
-	BUG_ON(start_pfn > last_pfn);
+	if (WARN_ON_ONCE(!domain_pfn_range_valid(domain, start_pfn, last_pfn))
+		return;
 
 	/* we don't need lock here; nobody else touches the iova range */
 	dma_pte_clear_level(domain, agaw_to_level(domain->agaw),
@@ -1272,7 +1279,7 @@ static void __iommu_flush_context(struct intel_iommu *iommu,
 			| DMA_CCMD_SID(source_id) | DMA_CCMD_FM(function_mask);
 		break;
 	default:
-		BUG();
+		WARN_ON_ONCE(1);
 	}
 	val |= DMA_CCMD_ICC;
 
@@ -1308,7 +1315,7 @@ static void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
 		val_iva = size_order | addr;
 		break;
 	default:
-		BUG();
+		WARN_ON_ONCE(1);
 	}
 	/* Note: set drain read/write */
 #if 0
@@ -1508,7 +1515,8 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
 	u16 did = domain_id_iommu(domain, iommu);
 
-	BUG_ON(pages == 0);
+	if (WARN_ON_ONCE(pages == 0))
+		return;
 
 	if (ih)
 		ih = 1 << 6;
@@ -1930,7 +1938,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	pr_debug("Set context mapping for %02x:%02x.%d\n",
 		bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 
-	BUG_ON(!domain->pgd);
+	if (WARN_ON_ONCE(!domain->pgd))
+		return -EINVAL;
 
 	spin_lock(&iommu->lock);
 	ret = -ENOMEM;
@@ -2183,7 +2192,8 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	phys_addr_t pteval;
 	u64 attr;
 
-	BUG_ON(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1));
+	if (WARN_ON_ONCE(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1)))
+		return -EINVAL;
 
 	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
 		return -EINVAL;
@@ -4339,8 +4349,9 @@ static size_t intel_iommu_unmap(struct iommu_domain *domain,
 
 	/* Cope with horrid API which requires us to unmap more than the
 	   size argument if it happens to be a large-page mapping. */
-	BUG_ON(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level,
-			       GFP_ATOMIC));
+	if (WARN_ON_ONCE(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level,
+					 GFP_ATOMIC)))
+		return 0;
 
 	if (size < VTD_PAGE_SIZE << level_to_offset_bits(level))
 		size = VTD_PAGE_SIZE << level_to_offset_bits(level);
-- 
2.34.1

Re: [PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range callback

[Baolu Lu]

On 2023/3/29 20:46, Tina Zhang wrote:
> Add the missing check to avoid null pointer dereference.
> 
> The patch fixes below issue reported by klocwork tool:
> Pointer 'info' returned from call to function 'dev_iommu_priv_get'
> at line 180 may be NULL and may be dereferenced at line 186.
> 
> This patch can fix a potential null pointer dereference issue of
> releasing a device working in svm mode.

The svm logic guarantees that __flush_svm_range_dev() only be called
after iommu_sva_bind_device() and before iommu_sva_unbind_device(). Thus
@info should never be NULL. There's no need to add this check.

Best regards,
baolu

> 
> Reported-by: Yongwei Ma <yongwei.ma@intel.com>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/svm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 7367f56c3bad..837c1a4642e7 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -179,7 +179,7 @@ static void __flush_svm_range_dev(struct intel_svm *svm,
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(sdev->dev);
>   
> -	if (WARN_ON(!pages))
> +	if (WARN_ON(!info || !pages))
>   		return;
>   
>   	qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, pages, ih);

Re: [PATCH 2/4] iommu/vt-d: Fix operand size in bitwise operation

[Baolu Lu]

On 2023/3/29 20:46, Tina Zhang wrote:
> The patch fixes the klocwork issues that operands in a bitwise operation have
> different size at line 1692 of dmar.c, line 1898 and line 1907 of iommu.c.
> 
> Reported-by: Yongwei Ma <yongwei.ma@intel.com>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/dmar.c  | 2 +-
>   drivers/iommu/intel/iommu.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 6acfe879589c..7c77a59889ea 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1689,7 +1689,7 @@ static void __dmar_enable_qi(struct intel_iommu *iommu)
>   	 * is present.
>   	 */
>   	if (ecap_smts(iommu->ecap))
> -		val |= (1 << 11) | 1;
> +		val |= (1ull << 11) | 1ull;

How about BIT_ULL(11) | BIT_ULL(0)?

Ditto to other changes.

Best regards,
baolu

>   
>   	raw_spin_lock_irqsave(&iommu->register_lock, flags);
>   
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 7c2f4bd33582..085d31a77f6f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1895,7 +1895,7 @@ context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
>    */
>   static inline void context_set_sm_dte(struct context_entry *context)
>   {
> -	context->lo |= (1 << 2);
> +	context->lo |= (1ull << 2);
>   }
>   
>   /*
> @@ -1904,7 +1904,7 @@ static inline void context_set_sm_dte(struct context_entry *context)
>    */
>   static inline void context_set_sm_pre(struct context_entry *context)
>   {
> -	context->lo |= (1 << 4);
> +	context->lo |= (1ull << 4);
>   }
>   
>   /* Convert value to context PASID directory size field coding. */

Re: [PATCH 3/4] iommu/vt-d: Fix quoted string split across lines

[Baolu Lu]

On 2023/3/29 20:46, Tina Zhang wrote:
> This patch resolves checkpatch warnings for intel iommu driver.
> 
> WARNING: quoted string split across lines
> 	pr_err("%s: iommu width (%d) is not "
> 		"sufficient for the mapped address (%llx)\n",

We should have avoided this code style at the beginning. But it already
appears in Linus's tree. It doesn't make more sense to fix it now.

Best regards,
baolu

> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 085d31a77f6f..286bb17973a4 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4294,8 +4294,7 @@ static int intel_iommu_map(struct iommu_domain *domain,
>   		/* check if minimum agaw is sufficient for mapped address */
>   		end = __DOMAIN_MAX_ADDR(dmar_domain->gaw) + 1;
>   		if (end < max_addr) {
> -			pr_err("%s: iommu width (%d) is not "
> -			       "sufficient for the mapped address (%llx)\n",
> +			pr_err("%s: iommu width (%d) is not sufficient for the mapped address (%llx)\n",
>   			       __func__, dmar_domain->gaw, max_addr);
>   			return -EFAULT;
>   		}

Re: [PATCH 4/4] iommu/vt-d: Replace BUG()/BUG_ON() with WARN_ON/WARN_ON_ONCE()

[Baolu Lu]

On 2023/3/29 20:46, Tina Zhang wrote:
> Remove legacy BUG()/BUG_ON() and turn them into WARN_ON/WARN_ON_ONCE()
> with errors.

Yes. WARN() is better than BUG().

Best regards,
baolu

> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/intel/dmar.c  |  6 ++++--
>   drivers/iommu/intel/iommu.c | 39 ++++++++++++++++++++++++-------------
>   2 files changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 7c77a59889ea..39b0bad000ea 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -127,7 +127,8 @@ dmar_alloc_pci_notify_info(struct pci_dev *dev, unsigned long event)
>   	struct pci_dev *tmp;
>   	struct dmar_pci_notify_info *info;
>   
> -	BUG_ON(dev->is_virtfn);
> +	if (WARN_ON(dev->is_virtfn))
> +		return NULL;
>   
>   	/*
>   	 * Ignore devices that have a domain number higher than what can
> @@ -264,7 +265,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
>   						   get_device(dev));
>   				return 1;
>   			}
> -		BUG_ON(i >= devices_cnt);
> +		if (WARN_ON_ONCE(i >= devices_cnt))
> +			return -EINVAL;
>   	}
>   
>   	return 0;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 286bb17973a4..20f7a9b3a6a7 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -391,6 +391,14 @@ static inline int domain_pfn_supported(struct dmar_domain *domain,
>   	return !(addr_width < BITS_PER_LONG && pfn >> addr_width);
>   }
>   
> +static inline bool domain_pfn_range_valid(struct dmar_domain *domain,
> +					  unsigned long start_pfn,
> +					  unsigned long last_pfn)
> +{
> +	return (!domain_pfn_supported(domain, last_pfn)) ? false :
> +		(start_pfn > last_pfn) ? false : true;
> +}
> +
>   /*
>    * Calculate the Supported Adjusted Guest Address Widths of an IOMMU.
>    * Refer to 11.4.2 of the VT-d spec for the encoding of each bit of
> @@ -915,7 +923,8 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>   	int level = agaw_to_level(domain->agaw);
>   	int offset;
>   
> -	BUG_ON(!domain->pgd);
> +	if (WARN_ON_ONCE(!domain->pgd))
> +		return NULL;
>   
>   	if (!domain_pfn_supported(domain, pfn))
>   		/* Address beyond IOMMU's addressing capabilities. */
> @@ -1005,9 +1014,8 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
>   	unsigned int large_page;
>   	struct dma_pte *first_pte, *pte;
>   
> -	BUG_ON(!domain_pfn_supported(domain, start_pfn));
> -	BUG_ON(!domain_pfn_supported(domain, last_pfn));
> -	BUG_ON(start_pfn > last_pfn);
> +	if (WARN_ON_ONCE(!domain_pfn_range_valid(domain, start_pfn, last_pfn))
> +		return;
>   
>   	/* we don't need lock here; nobody else touches the iova range */
>   	do {
> @@ -1166,9 +1174,8 @@ static void dma_pte_clear_level(struct dmar_domain *domain, int level,
>   static void domain_unmap(struct dmar_domain *domain, unsigned long start_pfn,
>   			 unsigned long last_pfn, struct list_head *freelist)
>   {
> -	BUG_ON(!domain_pfn_supported(domain, start_pfn));
> -	BUG_ON(!domain_pfn_supported(domain, last_pfn));
> -	BUG_ON(start_pfn > last_pfn);
> +	if (WARN_ON_ONCE(!domain_pfn_range_valid(domain, start_pfn, last_pfn))
> +		return;
>   
>   	/* we don't need lock here; nobody else touches the iova range */
>   	dma_pte_clear_level(domain, agaw_to_level(domain->agaw),
> @@ -1272,7 +1279,7 @@ static void __iommu_flush_context(struct intel_iommu *iommu,
>   			| DMA_CCMD_SID(source_id) | DMA_CCMD_FM(function_mask);
>   		break;
>   	default:
> -		BUG();
> +		WARN_ON_ONCE(1);
>   	}
>   	val |= DMA_CCMD_ICC;
>   
> @@ -1308,7 +1315,7 @@ static void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
>   		val_iva = size_order | addr;
>   		break;
>   	default:
> -		BUG();
> +		WARN_ON_ONCE(1);
>   	}
>   	/* Note: set drain read/write */
>   #if 0
> @@ -1508,7 +1515,8 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
>   	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
>   	u16 did = domain_id_iommu(domain, iommu);
>   
> -	BUG_ON(pages == 0);
> +	if (WARN_ON_ONCE(pages == 0))
> +		return;
>   
>   	if (ih)
>   		ih = 1 << 6;
> @@ -1930,7 +1938,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   	pr_debug("Set context mapping for %02x:%02x.%d\n",
>   		bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>   
> -	BUG_ON(!domain->pgd);
> +	if (WARN_ON_ONCE(!domain->pgd))
> +		return -EINVAL;
>   
>   	spin_lock(&iommu->lock);
>   	ret = -ENOMEM;
> @@ -2183,7 +2192,8 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
>   	phys_addr_t pteval;
>   	u64 attr;
>   
> -	BUG_ON(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1));
> +	if (WARN_ON_ONCE(!domain_pfn_supported(domain, iov_pfn + nr_pages - 1)))
> +		return -EINVAL;
>   
>   	if ((prot & (DMA_PTE_READ|DMA_PTE_WRITE)) == 0)
>   		return -EINVAL;
> @@ -4339,8 +4349,9 @@ static size_t intel_iommu_unmap(struct iommu_domain *domain,
>   
>   	/* Cope with horrid API which requires us to unmap more than the
>   	   size argument if it happens to be a large-page mapping. */
> -	BUG_ON(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level,
> -			       GFP_ATOMIC));
> +	if (WARN_ON_ONCE(!pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &level,
> +					 GFP_ATOMIC)))
> +		return 0;
>   
>   	if (size < VTD_PAGE_SIZE << level_to_offset_bits(level))
>   		size = VTD_PAGE_SIZE << level_to_offset_bits(level);

RE: [PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range callback

[Zhang, Tina]

Hi,
> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, March 29, 2023 9:31 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Ma, Yongwei <yongwei.ma@intel.com>
> Subject: Re: [PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range
> callback
> 
> On 2023/3/29 20:46, Tina Zhang wrote:
> > Add the missing check to avoid null pointer dereference.
> >
> > The patch fixes below issue reported by klocwork tool:
> > Pointer 'info' returned from call to function 'dev_iommu_priv_get'
> > at line 180 may be NULL and may be dereferenced at line 186.
> >
> > This patch can fix a potential null pointer dereference issue of
> > releasing a device working in svm mode.
> 
> The svm logic guarantees that __flush_svm_range_dev() only be called after
> iommu_sva_bind_device() and before iommu_sva_unbind_device(). Thus @info
> should never be NULL. There's no need to add this check.
Yes, if the order can be ensured, no need to worry about the null pointer. How about hot-plugging devices? Can we keep the order all the time? Otherwise, it seems adding a null check makes sense.

Regards,
-Tina


> 
> Best regards,
> baolu
> 
> >
> > Reported-by: Yongwei Ma <yongwei.ma@intel.com>
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/svm.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> > index 7367f56c3bad..837c1a4642e7 100644
> > --- a/drivers/iommu/intel/svm.c
> > +++ b/drivers/iommu/intel/svm.c
> > @@ -179,7 +179,7 @@ static void __flush_svm_range_dev(struct intel_svm
> *svm,
> >   {
> >   	struct device_domain_info *info = dev_iommu_priv_get(sdev->dev);
> >
> > -	if (WARN_ON(!pages))
> > +	if (WARN_ON(!info || !pages))
> >   		return;
> >
> >   	qi_flush_piotlb(sdev->iommu, sdev->did, svm->pasid, address, pages,
> > ih);

RE: [PATCH 2/4] iommu/vt-d: Fix operand size in bitwise operation

[Zhang, Tina]

Hi,
> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, March 29, 2023 9:33 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com; Ma, Yongwei <yongwei.ma@intel.com>
> Subject: Re: [PATCH 2/4] iommu/vt-d: Fix operand size in bitwise operation
> 
> On 2023/3/29 20:46, Tina Zhang wrote:
> > The patch fixes the klocwork issues that operands in a bitwise
> > operation have different size at line 1692 of dmar.c, line 1898 and line 1907 of
> iommu.c.
> >
> > Reported-by: Yongwei Ma <yongwei.ma@intel.com>
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/dmar.c  | 2 +-
> >   drivers/iommu/intel/iommu.c | 4 ++--
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> > index 6acfe879589c..7c77a59889ea 100644
> > --- a/drivers/iommu/intel/dmar.c
> > +++ b/drivers/iommu/intel/dmar.c
> > @@ -1689,7 +1689,7 @@ static void __dmar_enable_qi(struct intel_iommu
> *iommu)
> >   	 * is present.
> >   	 */
> >   	if (ecap_smts(iommu->ecap))
> > -		val |= (1 << 11) | 1;
> > +		val |= (1ull << 11) | 1ull;
> 
> How about BIT_ULL(11) | BIT_ULL(0)?
Thanks for the advice. I didn't know they are more popular 😊. Anyway, I guess we can try them instead.

Regards,
-Tina
> 
> Ditto to other changes.
> 
> Best regards,
> baolu
> 
> >
> >   	raw_spin_lock_irqsave(&iommu->register_lock, flags);
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 7c2f4bd33582..085d31a77f6f 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1895,7 +1895,7 @@ context_set_sm_rid2pasid(struct context_entry
> *context, unsigned long pasid)
> >    */
> >   static inline void context_set_sm_dte(struct context_entry *context)
> >   {
> > -	context->lo |= (1 << 2);
> > +	context->lo |= (1ull << 2);
> >   }
> >
> >   /*
> > @@ -1904,7 +1904,7 @@ static inline void context_set_sm_dte(struct
> context_entry *context)
> >    */
> >   static inline void context_set_sm_pre(struct context_entry *context)
> >   {
> > -	context->lo |= (1 << 4);
> > +	context->lo |= (1ull << 4);
> >   }
> >
> >   /* Convert value to context PASID directory size field coding. */

Re: [PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range callback

[Baolu Lu]

On 2023/3/29 21:40, Zhang, Tina wrote:
>> -----Original Message-----
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, March 29, 2023 9:31 PM
>> To: Zhang, Tina<tina.zhang@intel.com>;iommu@lists.linux.dev
>> Cc:baolu.lu@linux.intel.com; Ma, Yongwei<yongwei.ma@intel.com>
>> Subject: Re: [PATCH 1/4] iommu/vt-d: Fix null pointer access in invalidate_range
>> callback
>>
>> On 2023/3/29 20:46, Tina Zhang wrote:
>>> Add the missing check to avoid null pointer dereference.
>>>
>>> The patch fixes below issue reported by klocwork tool:
>>> Pointer 'info' returned from call to function 'dev_iommu_priv_get'
>>> at line 180 may be NULL and may be dereferenced at line 186.
>>>
>>> This patch can fix a potential null pointer dereference issue of
>>> releasing a device working in svm mode.
>> The svm logic guarantees that __flush_svm_range_dev() only be called after
>> iommu_sva_bind_device() and before iommu_sva_unbind_device(). Thus @info
>> should never be NULL. There's no need to add this check.
> Yes, if the order can be ensured, no need to worry about the null pointer. How about hot-plugging devices? Can we keep the order all the time? Otherwise, it seems adding a null check makes sense.

Removing a device always happens after driver unbinding. The driver
should unbind sva in its release path.

Best regards,
baolu

RE: [PATCH 3/4] iommu/vt-d: Fix quoted string split across lines

[Zhang, Tina]

Hi,

> -----Original Message-----
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, March 29, 2023 9:38 PM
> To: Zhang, Tina <tina.zhang@intel.com>; iommu@lists.linux.dev
> Cc: baolu.lu@linux.intel.com
> Subject: Re: [PATCH 3/4] iommu/vt-d: Fix quoted string split across lines
> 
> On 2023/3/29 20:46, Tina Zhang wrote:
> > This patch resolves checkpatch warnings for intel iommu driver.
> >
> > WARNING: quoted string split across lines
> > 	pr_err("%s: iommu width (%d) is not "
> > 		"sufficient for the mapped address (%llx)\n",
> 
> We should have avoided this code style at the beginning. But it already appears
> in Linus's tree. It doesn't make more sense to fix it now.
Right. We saw this issue when we tried to move the logic into another file dedicated for io page table handling. OK. I'll cherry-pick this patch into that series.

Regards,
-Tina
> 
> Best regards,
> baolu
> 
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 085d31a77f6f..286bb17973a4 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4294,8 +4294,7 @@ static int intel_iommu_map(struct iommu_domain
> *domain,
> >   		/* check if minimum agaw is sufficient for mapped address */
> >   		end = __DOMAIN_MAX_ADDR(dmar_domain->gaw) + 1;
> >   		if (end < max_addr) {
> > -			pr_err("%s: iommu width (%d) is not "
> > -			       "sufficient for the mapped address (%llx)\n",
> > +			pr_err("%s: iommu width (%d) is not sufficient for the
> mapped
> > +address (%llx)\n",
> >   			       __func__, dmar_domain->gaw, max_addr);
> >   			return -EFAULT;
> >   		}