在 2021/4/16 16:53, John Garry 写道:

On 16/04/2021 04:30, chenxiang (M) wrote:

...

...

you need to make this: if (iova) free_iova_mem(iova);

as free_iova_mem(iova) dereferences iova:

if (iova->pfn_lo != IOVA_ANCHOR) kmem_cache_free(iova_cache, iova)

So if iova were NULL, we crash.

Or you can make free_iova_mem() NULL safe.

Right, it has the issue. What about changing it as below?

@@ -472,10 +472,13 @@ free_iova(struct iova_domain *iovad, unsigned long pfn)

     spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
     iova = private_find_iova(iovad, pfn);

•   if (iova)
  

•           private_free_iova(iovad, iova);
  

•   if (!iova) {
  

• spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);

•           return;
  

•   }
  

•   remove_iova(iovad, iova);
     spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
  

•   free_iova_mem(iova);
  

  } EXPORT_SYMBOL_GPL(free_iova);

I don't feel too strongly about how it's done.

Please note that kmem_cache_free() is not NULL safe. So the NULL check could be added in free_iova_mem(), but we prob don't want silent free_iova_mem(NULL) calls, so I would stick with changing free_iova().

so I would stick with changing free_iova() ----- Do you mean free_iova_mem()?

But i think there is a check (judge iova is NULL) before free_iova_mem() for following scenarios, and it is not necessary to change in funciton free_iova_mem(): 1) iova_magazine_free_pfns() 2) alloc_iova() 3) free_iova() 4) __free_iova(): Those functions which call __free_iova() have a check

Only for function put_iova_doamin(), it doesn't check iova, but i think iova in rbtree should not be NULL.

Thanks, John

.