linux-kernel - Re: [PATCH 08/10] dmapool: cleanup dma_pool

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 20:33:23 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Tony Battersby <tonyb@...ernetics.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc:     iommu@...ts.linux-foundation.org, kernel-team@...com,
        Matthew Wilcox <willy@...radead.org>,
        Keith Busch <kbusch@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Tony Lindgren <tony@...mide.com>
Subject: Re: [PATCH 08/10] dmapool: cleanup dma_pool_destroy

On 2022-05-31 19:22, Tony Battersby wrote:
> Remove a small amount of code duplication between dma_pool_destroy() and
> pool_free_page() in preparation for adding more code without having to
> duplicate it.  No functional changes.
> 
> Signed-off-by: Tony Battersby <tonyb@...ernetics.com>
> ---
>   mm/dmapool.c | 34 ++++++++++++++++++++--------------
>   1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index 8749a9d7927e..58c11dcaa4e4 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -250,14 +250,25 @@ static inline bool is_page_busy(struct dma_page *page)
>   	return page->in_use != 0;
>   }
>   
> -static void pool_free_page(struct dma_pool *pool, struct dma_page *page)
> +static void pool_free_page(struct dma_pool *pool,
> +			   struct dma_page *page,
> +			   bool destroying_pool)
>   {
> +	void *vaddr = page->vaddr;
>   	dma_addr_t dma = page->dma;
>   
> +	if (destroying_pool && is_page_busy(page)) {
> +		dev_err(pool->dev,
> +			"dma_pool_destroy %s, %p busy\n",
> +			pool->name, vaddr);
> +		/* leak the still-in-use consistent memory */
> +	} else {
>   #ifdef	DMAPOOL_DEBUG
> -	memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
> +		memset(vaddr, POOL_POISON_FREED, pool->allocation);
>   #endif
> -	dma_free_coherent(pool->dev, pool->allocation, page->vaddr, dma);
> +		dma_free_coherent(pool->dev, pool->allocation, vaddr, dma);
> +	}
> +
>   	list_del(&page->page_list);

If we're tearing down the whole pool, surely we can skip this as well? 
(Same for the second list in patch #9)

In fact I think it might make more sense to refactor in the opposite 
direction and just streamline this directly into dma_pool_destroy(), 
more like:

	list_for_each_entry_safe() {
		if (is_page_busy()) {
			dev_err();
		} else {
			dma_free_coherent();
		}
		kfree(page);
	}

>   	kfree(page);
>   }
> @@ -272,7 +283,7 @@ static void pool_free_page(struct dma_pool *pool, struct dma_page *page)
>    */
>   void dma_pool_destroy(struct dma_pool *pool)
>   {
> -	struct dma_page *page, *tmp;
> +	struct dma_page *page;

Nit: you bring this back again in patch #10, so we may as well leave the 
list_for_each_entry_safe() iterator in place until then as well, and 
save a bit of churn in this patch.

>   	bool empty = false;
>   
>   	if (unlikely(!pool))
> @@ -288,15 +299,10 @@ void dma_pool_destroy(struct dma_pool *pool)
>   		device_remove_file(pool->dev, &dev_attr_pools);
>   	mutex_unlock(&pools_reg_lock);
>   
> -	list_for_each_entry_safe(page, tmp, &pool->page_list, page_list) {
> -		if (is_page_busy(page)) {
> -			dev_err(pool->dev, "%s %s, %p busy\n", __func__,
> -				pool->name, page->vaddr);
> -			/* leak the still-in-use consistent memory */
> -			list_del(&page->page_list);
> -			kfree(page);
> -		} else
> -			pool_free_page(pool, page);
> +	while ((page = list_first_entry_or_null(&pool->page_list,
> +						struct dma_page,
> +						page_list))) {
> +		pool_free_page(pool, page, true);
>   	}
>   
>   	kfree(pool);
> @@ -469,7 +475,7 @@ void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
>   	page->offset = offset;
>   	/*
>   	 * Resist a temptation to do
> -	 *    if (!is_page_busy(page)) pool_free_page(pool, page);
> +	 *    if (!is_page_busy(page)) pool_free_page(pool, page, false);

Further to the above, even if we did retain a separate function, if an 
argument is hard-coded at the one single callsite, and the only 
reference to passing any other value is in a comment effectively saying 
"don't do this", do we really need to pretend it's an argument at all? ;)

FWIW I'd just reword the comment in more general terms, e.g. "Resist the 
temptation to free unused pages immediately..."

Thanks,
Robin.

>   	 * Better have a few empty pages hang around.
>   	 */
>   	spin_unlock_irqrestore(&pool->lock, flags);