linux-kernel - Re: [PATCH 1/2] iommu/iova: Flush CPU rcache for when a depot fills

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 25 Sep 2020 17:12:53 +0100
From:   John Garry <john.garry@...wei.com>
To:     Robin Murphy <robin.murphy@....com>, <joro@...tes.org>
CC:     <linuxarm@...wei.com>, <linux-kernel@...r.kernel.org>,
        <iommu@...ts.linux-foundation.org>, <xiyou.wangcong@...il.com>
Subject: Re: [PATCH 1/2] iommu/iova: Flush CPU rcache for when a depot fills

On 25/09/2020 15:34, John Garry wrote:
> Indeed, I think that the mainline code has a bug:
> 
> If the initial allocation for the loaded/prev magazines fail (give NULL) 
> in init_iova_rcaches(), then in __iova_rcache_insert():
> 
> if (!iova_magazine_full(cpu_rcache->loaded)) {
>      can_insert = true;
> 
> If cpu_rcache->loaded == NULL, then can_insert is assigned true -> bang, 
> as I experimented, below. This needs to be fixed...
> 

This looks better:

Subject: [PATCH] iommu/iova: Avoid double-negatives with magazine helpers

Expression !iova_magazine_full(mag) evaluates true when mag == NULL.

This falls over in __iova_rcache_insert() when loaded == NULL:

if (!iova_magazine_full(cpu_rcache->loaded)) {
	can_insert = true;

...

if (can_insert)
	iova_magazine_push(cpu_rcache->loaded, iova_pfn);

Here, can_insert is evaluated true, which is wrong. Members
loaded/prev can possibly be NULL if the initial allocations fail in
__iova_rcache_insert().

Let's stop using double-negatives, like !iova_magazine_full(), and use
iova_magazine_has_space() instead in this case. And similar for
!iova_magazine_empty().

Signed-off-by: John Garry <john.garry@...wei.com>

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 5b4ffab7140b..42ca9d0f39b7 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -827,14 +827,18 @@ iova_magazine_free_pfns(struct iova_magazine *mag, 
struct iova_domain *iovad)
  	mag->size = 0;
  }

-static bool iova_magazine_full(struct iova_magazine *mag)
+static bool iova_magazine_has_space(struct iova_magazine *mag)
  {
-	return (mag && mag->size == IOVA_MAG_SIZE);
+	if (!mag)
+		return false;
+	return mag->size < IOVA_MAG_SIZE;
  }

-static bool iova_magazine_empty(struct iova_magazine *mag)
+static bool iova_magazine_has_pfns(struct iova_magazine *mag)
  {
-	return (!mag || mag->size == 0);
+	if (!mag)
+		return false;
+	return mag->size;
  }

  static unsigned long iova_magazine_pop(struct iova_magazine *mag,
@@ -843,7 +847,7 @@ static unsigned long iova_magazine_pop(struct 
iova_magazine *mag,
  	int i;
  	unsigned long pfn;

-	BUG_ON(iova_magazine_empty(mag));
+	BUG_ON(!iova_magazine_has_pfns(mag));

  	/* Only fall back to the rbtree if we have no suitable pfns at all */
  	for (i = mag->size - 1; mag->pfns[i] > limit_pfn; i--)
@@ -859,7 +863,7 @@ static unsigned long iova_magazine_pop(struct 
iova_magazine *mag,

  static void iova_magazine_push(struct iova_magazine *mag, unsigned 
long pfn)
  {
-	BUG_ON(iova_magazine_full(mag));
+	BUG_ON(!iova_magazine_has_space(mag));

  	mag->pfns[mag->size++] = pfn;
  }
@@ -905,9 +909,9 @@ static bool __iova_rcache_insert(struct iova_domain 
*iovad,
  	cpu_rcache = raw_cpu_ptr(rcache->cpu_rcaches);
  	spin_lock_irqsave(&cpu_rcache->lock, flags);

-	if (!iova_magazine_full(cpu_rcache->loaded)) {
+	if (iova_magazine_has_space(cpu_rcache->loaded)) {
  		can_insert = true;
-	} else if (!iova_magazine_full(cpu_rcache->prev)) {
+	} else if (iova_magazine_has_space(cpu_rcache->prev)) {
  		swap(cpu_rcache->prev, cpu_rcache->loaded);
  		can_insert = true;
  	} else {
@@ -915,7 +919,8 @@ static bool __iova_rcache_insert(struct iova_domain 
*iovad,

  		if (new_mag) {
  			spin_lock(&rcache->lock);
-			if (rcache->depot_size < MAX_GLOBAL_MAGS) {
+			if (rcache->depot_size < MAX_GLOBAL_MAGS &&
+			    cpu_rcache->loaded) {
  				rcache->depot[rcache->depot_size++] =
  						cpu_rcache->loaded;
  			} else {
@@ -968,9 +973,9 @@ static unsigned long __iova_rcache_get(struct 
iova_rcache *rcache,
  	cpu_rcache = raw_cpu_ptr(rcache->cpu_rcaches);
  	spin_lock_irqsave(&cpu_rcache->lock, flags);

-	if (!iova_magazine_empty(cpu_rcache->loaded)) {
+	if (iova_magazine_has_pfns(cpu_rcache->loaded)) {
  		has_pfn = true;
-	} else if (!iova_magazine_empty(cpu_rcache->prev)) {
+	} else if (iova_magazine_has_pfns(cpu_rcache->prev)) {
  		swap(cpu_rcache->prev, cpu_rcache->loaded);
  		has_pfn = true;
  	} else {
-- 
2.26.2