linux-kernel - Re: [PATCH net v2 2/2] 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: <934d601f-be43-4e04-b126-dc86890a4bfa@huawei.com>
Date: Fri, 27 Sep 2024 19:29:22 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
CC: Mina Almasry <almasrymina@...gle.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <liuyonglong@...wei.com>,
	<fanghaiqing@...wei.com>, <zhangkun09@...wei.com>, Robin Murphy
	<robin.murphy@....com>, Alexander Duyck <alexander.duyck@...il.com>, IOMMU
	<iommu@...ts.linux.dev>, Wei Fang <wei.fang@....com>, Shenwei Wang
	<shenwei.wang@....com>, Clark Wang <xiaoning.wang@....com>, Eric Dumazet
	<edumazet@...gle.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, Przemek
 Kitszel <przemyslaw.kitszel@...el.com>, Alexander Lobakin
	<aleksander.lobakin@...el.com>, Alexei Starovoitov <ast@...nel.org>, Daniel
 Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>, Saeed Mahameed
	<saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>, Tariq Toukan
	<tariqt@...dia.com>, Felix Fietkau <nbd@....name>, Lorenzo Bianconi
	<lorenzo@...nel.org>, Ryder Lee <ryder.lee@...iatek.com>, Shayne Chen
	<shayne.chen@...iatek.com>, Sean Wang <sean.wang@...iatek.com>, Kalle Valo
	<kvalo@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Andrew
 Morton <akpm@...ux-foundation.org>, <imx@...ts.linux.dev>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <bpf@...r.kernel.org>,
	<linux-rdma@...r.kernel.org>, <linux-wireless@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-mediatek@...ts.infradead.org>,
	<linux-mm@...ck.org>
Subject: Re: [PATCH net v2 2/2] page_pool: fix IOMMU crash when driver has
 already unbound

On 2024/9/27 17:58, Ilias Apalodimas wrote:

...

>>
>>> importantly, though, why does struct page need to know about this?
>>> Can't we have the same information in page pool?
>>> When the driver allocates pages it does via page_pool_dev_alloc_XXXXX
>>> or something similar. Cant we do what you suggest here ? IOW when we
>>> allocate a page we put it in a list, and when that page returns to
>>> page_pool (and it's mapped) we remove it.
>>
>> Yes, that is the basic idea, but the important part is how to do that
>> with less performance impact.
> 
> Yes, but do you think that keeping that list of allocated pages in
> struct page_pool will end up being more costly somehow compared to
> struct page?

I am not sure if I understand your above question here.
I am supposing the question is about what's the cost between using
single/doubly linked list for the inflight pages or using a array
for the inflight pages like this patch does using pool->items?
If I understand question correctly, the single/doubly linked list
is more costly than array as the page_pool case as my understanding.

For single linked list, it doesn't allow deleting a specific entry but
only support deleting the first entry and all the entries. It does support
lockless operation using llist, but have limitation as below:
https://elixir.bootlin.com/linux/v6.7-rc8/source/include/linux/llist.h#L13

For doubly linked list, it needs two pointer to support deleting a specific
entry and it does not support lockless operation.

For pool->items, as the alloc side is protected by NAPI context, and the
free side use item->pp_idx to ensure there is only one producer for each
item, which means for each item in pool->items, there is only one consumer
and one producer, which seems much like the case when the page is not
recyclable in __page_pool_put_page, we don't need a lock protection when
calling page_pool_return_page(), the 'struct page' is also one consumer
and one producer as the pool->items[item->pp_idx] does:
https://elixir.bootlin.com/linux/v6.7-rc8/source/net/core/page_pool.c#L645

We only need a lock protection when page_pool_destroy() is called to
check if there is inflight page to be unmapped as a consumer, and the
__page_pool_put_page() may also called to unmapped the inflight page as
another consumer, there is why the 'destroy_lock' is added for protection
when pool->destroy_cnt > 0.

> 
> Thanks
> /Ilias