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
| ||
|
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
Powered by blists - more mailing lists