Re: [PATCH RFC v4 2/3] page_pool: fix IOMMU crash when driver has already unbound

[Yunsheng Lin]

On 2024/11/26 18:22, Jesper Dangaard Brouer wrote:

...

>>>
>>> Once the a page is release from a page pool it becomes a normal page,
>>> that adhere to normal page refcnt'ing. That is how it worked before with
>>> page_pool_release_page().
>>> The later extensions with page fragment support and devmem might have
>>> complicated this code path.
>>
>> As page_pool_return_page() and page_pool_destroy() both try to "release"
>> the page concurrently for a specific page, I am not sure how using some
>> simple *atomic* can avoid this kind of concurrency even before page
>> fragment and devmem are supported, it would be good to be more specific
>> about that by using some pseudocode.
>>
> 
> Okay, some my simple atomic idea will not work.
> 
> NEW IDEA:
> 
> So, the my concern in this patchset is that BH-disabling spin_lock pool->destroy_lock is held in the outer loop of page_pool_inflight_unmap() that scans all pages.  Disabling BH for this long have nasty side-effects.
> 
> Will it be enough to grab the pool->destroy_lock only when we detect a page that belongs to our page pool?  Of-cause after obtaining the lock. the code need to recheck if the page still belongs to the pool.
> 

That means there will be page_pool_return_page() called between the scanning,
it seems like a lot like the idea of 'page_pool_get_dma_addr() need to be
checked to decide if the mapping is already done or not for each page.' as
there are two cases when page_pool_return_page() is called during scanning:
1. page_pool_get_dma_addr() returns non-zero dma address, which means the dma
   unmapping is not done by scanning yet, page_pool_return_page() need to do
   the dma unmapping before calling put_page()
2. page_pool_get_dma_addr() returns zero dma address, which means the dma
   unmapping is done by scanning, page_pool_return_page() just skip the dma
   unmapping and only call put_page().

It seems there is only one case for scanning:
1. page_pool_get_dma_addr() for a page_pool owned page returns non-zero dma
   address, which means page_pool_return_page() is not called for that page yet,
   scanning will the do the mapping for page_pool_return_page() and reset the
   dma address of the page to indicate the dma unmapping is done for that page.

It seems there is no case of page_pool owned page having zero dma address during
scanning, as both page->pp_magic is cleared and dma unmapping is already done in
page_pool_return_page().