linux-kernel - Re: [PATCH v2] mm: cma: indefinitely retry allocations in cma

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]

Message-ID: <410a4e0c-f924-4564-ae1e-cc9f6292c88e@gmail.com>
Date:   Fri, 11 Sep 2020 14:37:28 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Chris Goldsworthy <cgoldswo@...eaurora.org>,
        linux-kernel@...r.kernel.org
Cc:     Vinayak Menon <vinmenon@...eaurora.org>
Subject: Re: [PATCH v2] mm: cma: indefinitely retry allocations in cma_alloc



On 9/11/2020 1:54 PM, Chris Goldsworthy wrote:
> CMA allocations will fail if 'pinned' pages are in a CMA area, since we
> cannot migrate pinned pages. The _refcount of a struct page being greater
> than _mapcount for that page can cause pinning for anonymous pages.  This
> is because try_to_unmap(), which (1) is called in the CMA allocation path,
> and (2) decrements both _refcount and _mapcount for a page, will stop
> unmapping a page from VMAs once the _mapcount for a page reaches 0.  This
> implies that after try_to_unmap() has finished successfully for a page
> where _recount > _mapcount, that _refcount will be greater than 0.  Later
> in the CMA allocation path in migrate_page_move_mapping(), we will have one
> more reference count than intended for anonymous pages, meaning the
> allocation will fail for that page.
> 
> One example of where _refcount can be greater than _mapcount for a page we
> would not expect to be pinned is inside of copy_one_pte(), which is called
> during a fork. For ptes for which pte_present(pte) == true, copy_one_pte()
> will increment the _refcount field followed by the  _mapcount field of a
> page. If the process doing copy_one_pte() is context switched out after
> incrementing _refcount but before incrementing _mapcount, then the page
> will be temporarily pinned.
> 
> So, inside of cma_alloc(), instead of giving up when alloc_contig_range()
> returns -EBUSY after having scanned a whole CMA-region bitmap, perform
> retries indefinitely, with sleeps, to give the system an opportunity to
> unpin any pinned pages.

I am by no means an authoritative CMA person but this behavior does not 
seem acceptable, there is no doubt the existing one is sub-optimal under 
specific circumstances, but an indefinite retry, as well as a 100ms 
sleep appear to be arbitrary at best. How about you introduce a 
parameter that allows the tuning of the number of retries and/or delay 
between retries?

> 
> Signed-off-by: Chris Goldsworthy <cgoldswo@...eaurora.org>
> Co-developed-by: Vinayak Menon <vinmenon@...eaurora.org>
> Signed-off-by: Vinayak Menon <vinmenon@...eaurora.org>
> ---
>   mm/cma.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/cma.c b/mm/cma.c
> index 7f415d7..90bb505 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -32,6 +32,7 @@
>   #include <linux/highmem.h>
>   #include <linux/io.h>
>   #include <linux/kmemleak.h>
> +#include <linux/delay.h>
>   #include <trace/events/cma.h>
>   
>   #include "cma.h"
> @@ -442,8 +443,28 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
>   				bitmap_maxno, start, bitmap_count, mask,
>   				offset);
>   		if (bitmap_no >= bitmap_maxno) {
> -			mutex_unlock(&cma->lock);
> -			break;
> +			if (ret == -EBUSY) {
> +				mutex_unlock(&cma->lock);
> +
> +				/*
> +				 * Page may be momentarily pinned by some other
> +				 * process which has been scheduled out, e.g.
> +				 * in exit path, during unmap call, or process
> +				 * fork and so cannot be freed there. Sleep
> +				 * for 100ms and retry the allocation.
> +				 */
> +				start = 0;
> +				ret = -ENOMEM;
> +				msleep(100);
> +				continue;
> +			} else {
> +				/*
> +				 * ret == -ENOMEM - all bits in cma->bitmap are
> +				 * set, so we break accordingly.
> +				 */
> +				mutex_unlock(&cma->lock);
> +				break;
> +			}
>   		}
>   		bitmap_set(cma->bitmap, bitmap_no, bitmap_count);
>   		/*
> 

-- 
Florian