netdev - Re: [PATCH net-next v7 3/8] page_pool: fix IOMMU crash when driver has already unbound

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <3795e5e1-8a6a-4ee3-9778-1828ac5b2748@gmail.com>
Date: Sat, 18 Jan 2025 21:36:52 +0800
From: Yunsheng Lin <yunshenglin0825@...il.com>
To: Jesper Dangaard Brouer <hawk@...nel.org>,
 Yunsheng Lin <linyunsheng@...wei.com>, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com
Cc: zhangkun09@...wei.com, liuyonglong@...wei.com, fanghaiqing@...wei.com,
 Robin Murphy <robin.murphy@....com>,
 Alexander Duyck <alexander.duyck@...il.com>, IOMMU <iommu@...ts.linux.dev>,
 Andrew Morton <akpm@...ux-foundation.org>, Eric Dumazet
 <edumazet@...gle.com>, Simon Horman <horms@...nel.org>,
 Ilias Apalodimas <ilias.apalodimas@...aro.org>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 kernel-team <kernel-team@...udflare.com>
Subject: Re: [PATCH net-next v7 3/8] page_pool: fix IOMMU crash when driver
 has already unbound

On 1/18/2025 12:56 AM, Jesper Dangaard Brouer wrote:

...

>> I am not really sure about that, as using the PAGE_SIZE block to hold the
>> item seems like a implementation detail, which might change in the 
>> future,
>> renaming other function to something like that doesn't seem right to 
>> me IMHO.
>>
>> Also the next patch will add page_pool_item_blk_add() to support 
>> unlimited
>> inflight pages, it seems a better name is needed for that too, perheps 
>> rename
>> page_pool_item_blk_add() to page_pool_dynamic_item_add()?
>>
> 
> Hmmm... not sure about this.
> I think I prefer page_pool_item_blk_add() over 
> page_pool_dynamic_item_add().
> 
>> For __page_pool_item_init(), perhaps just inline it back to 
>> page_pool_item_init()
>> as __page_pool_item_init() is only used by page_pool_item_init(), and 
>> both of them
>> are not really large function.
> 
> I like that you had a helper function. So, don't merge 
> __page_pool_item_init() into page_pool_item_init() just to avoid naming 
> it differently.

Any particular reason for the above suggestion?

After reusing the page_pool_item_uninit() to fix the similar
use-after-freed problem，it seems reasonable to not expose the
item_blcok as much as possible as item_blcok is really an
implementation detail that should be hidden as much as possible
IMHO.

If it is able to reused for supporting the unlimited item case,
then I am agreed that it might be better to refactor it out,
but it is not really reusable.

static int page_pool_item_init(struct page_pool *pool)
{
#define PAGE_POOL_MIN_INFLIGHT_ITEMS            512
         struct page_pool_item_block *block;
         int item_cnt;

         INIT_LIST_HEAD(&pool->item_blocks);
         init_llist_head(&pool->hold_items);
         init_llist_head(&pool->release_items);

         item_cnt = pool->p.pool_size * 2 + PP_ALLOC_CACHE_SIZE +
                 PAGE_POOL_MIN_INFLIGHT_ITEMS;
         for (; item_cnt > 0; item_cnt -= ITEMS_PER_PAGE) {
                 struct page *page;
                 unsigned int i;

                 page = alloc_pages_node(pool->p.nid, GFP_KERNEL, 0);
                 if (!page) {
                         page_pool_item_uninit(pool);
                         return -ENOMEM;
                 }

                 block = page_address(page);
                 block->pp = pool;
                 list_add(&block->list, &pool->item_blocks);

                 for (i = 0; i < ITEMS_PER_PAGE; i++) {
                         page_pool_item_init_state(&block->items[i]);
                         __llist_add(&block->items[i].lentry, 
&pool->hold_items);
                 }
         }

         return 0;
}

> 
> Let me be more explicit what I'm asking for:
> 
> IMHO you should rename:
>   - __page_pool_item_init() to __page_pool_item_block_init()
> and rename:
>   - page_pool_item_init() to page_pool_item_block_init()
> 
> I hope this make it more clear what I'm saying.
 > > --Jesper
>