[RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Dmitry Safonov]

find_iova() looks to be using a bad locking practice: it locks the
returned iova only for the search time.
And looking in code, the element can be removed from the tree and freed
under rbtree lock. That happens during memory hot-unplug and cleanup on
module removal.
Here I cleanup users of the function and delete it.

Dmitry Safonov (3):
  iommu/iova: Find and split iova under rbtree's lock
  iommu/iova: Make free_iova() atomic
  iommu/iova: Remove find_iova()

 drivers/iommu/intel-iommu.c | 14 +++----------
 drivers/iommu/iova.c        | 48 +++++++++++++++++----------------------------
 include/linux/iova.h        | 17 ++++------------
 3 files changed, 25 insertions(+), 54 deletions(-)

-- 
2.13.6

[RFC 1/3] iommu/iova: Find and split iova under rbtree's lock

[Dmitry Safonov]

find_iova() holds iova_rbtree_lock only during the traversing rbtree.
After the lock is released, returned iova may be freed (e.g., during
module's release).
Hold the spinlock during search and removal of iova from the rbtree,
eleminating possible use-after-free or/and double-free of iova.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/iommu/intel-iommu.c | 14 +++-----------
 drivers/iommu/iova.c        | 19 ++++++++++++-------
 include/linux/iova.h        | 10 ++++------
 3 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 14e4b3722428..494394ef0f92 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4530,19 +4530,11 @@ static int intel_iommu_memory_notifier(struct notifier_block *nb,
 			struct intel_iommu *iommu;
 			struct page *freelist;
 
-			iova = find_iova(&si_domain->iovad, start_vpfn);
+			iova = iova_split_and_pop(&si_domain->iovad, start_vpfn, last_vpfn);
 			if (iova == NULL) {
-				pr_debug("Failed get IOVA for PFN %lx\n",
-					 start_vpfn);
-				break;
-			}
-
-			iova = split_and_remove_iova(&si_domain->iovad, iova,
-						     start_vpfn, last_vpfn);
-			if (iova == NULL) {
-				pr_warn("Failed to split IOVA PFN [%lx-%lx]\n",
+				pr_warn("Failed to split & pop IOVA PFN [%lx-%lx]\n",
 					start_vpfn, last_vpfn);
-				return NOTIFY_BAD;
+				break;
 			}
 
 			freelist = domain_unmap(si_domain, iova->pfn_lo,
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 83fe2621effe..4b38eb507670 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -715,23 +715,27 @@ copy_reserved_iova(struct iova_domain *from, struct iova_domain *to)
 }
 EXPORT_SYMBOL_GPL(copy_reserved_iova);
 
-struct iova *
-split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
+struct iova *iova_split_and_pop(struct iova_domain *iovad,
 		      unsigned long pfn_lo, unsigned long pfn_hi)
 {
-	unsigned long flags;
 	struct iova *prev = NULL, *next = NULL;
+	unsigned long flags;
+	struct iova *iova;
 
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+	iova = private_find_iova(iovad, pfn_lo);
+	if (iova == NULL)
+		goto err_unlock;
+
 	if (iova->pfn_lo < pfn_lo) {
 		prev = alloc_and_init_iova(iova->pfn_lo, pfn_lo - 1);
 		if (prev == NULL)
-			goto error;
+			goto err_unlock;
 	}
 	if (iova->pfn_hi > pfn_hi) {
 		next = alloc_and_init_iova(pfn_hi + 1, iova->pfn_hi);
 		if (next == NULL)
-			goto error;
+			goto err_free;
 	}
 
 	__cached_rbnode_delete_update(iovad, iova);
@@ -749,10 +753,11 @@ split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
 
 	return iova;
 
-error:
-	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+err_free:
 	if (prev)
 		free_iova_mem(prev);
+err_unlock:
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 	return NULL;
 }
 
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 928442dda565..803472b77919 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -160,8 +160,8 @@ int init_iova_flush_queue(struct iova_domain *iovad,
 			  iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
-struct iova *split_and_remove_iova(struct iova_domain *iovad,
-	struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
+struct iova *iova_split_and_pop(struct iova_domain *iovad,
+		unsigned long pfn_lo, unsigned long pfn_hi);
 void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
 #else
 static inline int iova_cache_get(void)
@@ -253,10 +253,8 @@ static inline void put_iova_domain(struct iova_domain *iovad)
 {
 }
 
-static inline struct iova *split_and_remove_iova(struct iova_domain *iovad,
-						 struct iova *iova,
-						 unsigned long pfn_lo,
-						 unsigned long pfn_hi)
+static inline struct iova *iova_split_and_pop(struct iova_domain *iovad,
+		unsigned long pfn_lo, unsigned long pfn_hi)
 {
 	return NULL;
 }
-- 
2.13.6

[RFC 2/3] iommu/iova: Make free_iova() atomic

[Dmitry Safonov]

find_iova() grabs rbtree's spinlock only for the search time.
Nothing guaranties that returned iova still exist for __free_iova().
Prevent potential use-after-free and double-free by holding the spinlock
all the time iova is being searched and freed.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/iommu/iova.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 4b38eb507670..4c63d92afaf7 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -382,11 +382,14 @@ EXPORT_SYMBOL_GPL(__free_iova);
 void
 free_iova(struct iova_domain *iovad, unsigned long pfn)
 {
-	struct iova *iova = find_iova(iovad, pfn);
+	unsigned long flags;
+	struct iova *iova;
 
+	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+	iova = private_find_iova(iovad, pfn);
 	if (iova)
-		__free_iova(iovad, iova);
-
+		private_free_iova(iovad, iova);
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 }
 EXPORT_SYMBOL_GPL(free_iova);
 
-- 
2.13.6

[RFC 3/3] iommu/iova: Remove find_iova()

[Dmitry Safonov]

This function is potentially dangerous: nothing protects returned iova.
As there is no user in tree anymore, delete it.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Cc: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/iommu/iova.c | 20 --------------------
 include/linux/iova.h |  7 -------
 2 files changed, 27 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 4c63d92afaf7..4a568e28a633 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -336,26 +336,6 @@ static void private_free_iova(struct iova_domain *iovad, struct iova *iova)
 }
 
 /**
- * find_iova - finds an iova for a given pfn
- * @iovad: - iova domain in question.
- * @pfn: - page frame number
- * This function finds and returns an iova belonging to the
- * given doamin which matches the given pfn.
- */
-struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn)
-{
-	unsigned long flags;
-	struct iova *iova;
-
-	/* Take the lock so that no other thread is manipulating the rbtree */
-	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-	iova = private_find_iova(iovad, pfn);
-	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-	return iova;
-}
-EXPORT_SYMBOL_GPL(find_iova);
-
-/**
  * __free_iova - frees the given iova
  * @iovad: iova domain in question.
  * @iova: iova in question.
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 803472b77919..006911306a84 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -158,7 +158,6 @@ void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn);
 int init_iova_flush_queue(struct iova_domain *iovad,
 			  iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
-struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *iova_split_and_pop(struct iova_domain *iovad,
 		unsigned long pfn_lo, unsigned long pfn_hi);
@@ -243,12 +242,6 @@ static inline int init_iova_flush_queue(struct iova_domain *iovad,
 	return -ENODEV;
 }
 
-static inline struct iova *find_iova(struct iova_domain *iovad,
-				     unsigned long pfn)
-{
-	return NULL;
-}
-
 static inline void put_iova_domain(struct iova_domain *iovad)
 {
 }
-- 
2.13.6

Re: [RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Dmitry Safonov]

Hi,

Any opinion on that?
It looks to remove a source of possible issues and
has a nice diffstat.

2018-06-21 19:08 GMT+01:00 Dmitry Safonov <dima@arista.com>:
> find_iova() looks to be using a bad locking practice: it locks the
> returned iova only for the search time.
> And looking in code, the element can be removed from the tree and freed
> under rbtree lock. That happens during memory hot-unplug and cleanup on
> module removal.
> Here I cleanup users of the function and delete it.
>
> Dmitry Safonov (3):
>   iommu/iova: Find and split iova under rbtree's lock
>   iommu/iova: Make free_iova() atomic
>   iommu/iova: Remove find_iova()
>
>  drivers/iommu/intel-iommu.c | 14 +++----------
>  drivers/iommu/iova.c        | 48 +++++++++++++++++----------------------------
>  include/linux/iova.h        | 17 ++++------------
>  3 files changed, 25 insertions(+), 54 deletions(-)
>

Thanks,
             Dmitry

Re: [RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Joerg Roedel]

On Thu, Jun 21, 2018 at 07:08:20PM +0100, Dmitry Safonov wrote:
> find_iova() looks to be using a bad locking practice: it locks the
> returned iova only for the search time.  And looking in code, the
> element can be removed from the tree and freed under rbtree lock. That
> happens during memory hot-unplug and cleanup on module removal.  Here
> I cleanup users of the function and delete it.

But this is only a problem if more than one code-path uses tries to
handle a given iova at the same time, no?

Regards,

	Joerg

Re: [RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Dmitry Safonov]

On Fri, 2018-07-06 at 15:16 +0200, Joerg Roedel wrote:
> On Thu, Jun 21, 2018 at 07:08:20PM +0100, Dmitry Safonov wrote:
> > find_iova() looks to be using a bad locking practice: it locks the
> > returned iova only for the search time.  And looking in code, the
> > element can be removed from the tree and freed under rbtree lock.
> > That
> > happens during memory hot-unplug and cleanup on module
> > removal.  Here
> > I cleanup users of the function and delete it.
> 
> But this is only a problem if more than one code-path uses tries to
> handle a given iova at the same time, no?

Yes, as far as I can see, there are code-paths which may try to handle
it at the same time:
o memory notifiers for hot-unplug (intel-iommu.c)
o drivers unloading calls free_iova(), which in the result calls
  find_iova()
o I see at least one driver that frees iova during it's normal work
  too: scif_rma.c:scif_free_window_offset()

So, I decided to fix the interface while it's not widely used instead
of all callers.
Looks worth for me even as it's all corner-cases like unplugging the
memory.

Anyway, just found it while some college wrote a debug sysfs interface
for iovas and used find_iova().
So, if you think it's not worth to change - that's fine for me, but I
thought I'll nip this in the bud, preventing other people to misuse it.

-- 
Thanks,
             Dmitry

Re: [RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Joerg Roedel]

On Fri, Jul 06, 2018 at 03:10:47PM +0100, Dmitry Safonov wrote:
> Yes, as far as I can see, there are code-paths which may try to handle
> it at the same time:
> o memory notifiers for hot-unplug (intel-iommu.c)
> o drivers unloading calls free_iova(), which in the result calls
>   find_iova()
> o I see at least one driver that frees iova during it's normal work
>   too: scif_rma.c:scif_free_window_offset()

Yeah, but the IOVAs freed in the memory notifiers are just the ones for
the direct-mapped RMRR regions requested by firmware, not the IOVAs
allocated by any driver, so I think this shouldn't be a problem.


Regards,

	Joerg

Re: [RFC 0/3] iommu/iova: Unsafe locking in find_iova()

[Dmitry Safonov]

On Fri, 2018-07-06 at 17:13 +0200, Joerg Roedel wrote:
> On Fri, Jul 06, 2018 at 03:10:47PM +0100, Dmitry Safonov wrote:
> > Yes, as far as I can see, there are code-paths which may try to
> > handle
> > it at the same time:
> > o memory notifiers for hot-unplug (intel-iommu.c)
> > o drivers unloading calls free_iova(), which in the result calls
> >   find_iova()
> > o I see at least one driver that frees iova during it's normal work
> >   too: scif_rma.c:scif_free_window_offset()
> 
> Yeah, but the IOVAs freed in the memory notifiers are just the ones
> for
> the direct-mapped RMRR regions requested by firmware, not the IOVAs
> allocated by any driver, so I think this shouldn't be a problem.

Ok, than drop it, please.
I thought that safer API + nice diffstat can justify the change, but
whatever %)

-- 
Thanks,
             Dmitry