| Message ID | 20190925005214.27240-15-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Large pages in the page-cache | expand |
On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: > From: William Kucharski <william.kucharski@oracle.com> > > When we have the opportunity to use transparent huge pages to map a > file, we want to follow the same rules as DAX. > > Signed-off-by: William Kucharski <william.kucharski@oracle.com> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/huge_memory.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index cbe7d0619439..670a1780bd2f 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, > > if (addr) > goto out; > - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) > - goto out; > > addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); > if (addr) I think you reducing ASLR without any real indication that THP is relevant for the VMA. We need to know if any huge page allocation will be *attempted* for the VMA or the file.
> On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index cbe7d0619439..670a1780bd2f 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, >> >> if (addr) >> goto out; >> - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) >> - goto out; >> >> addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); >> if (addr) > > I think you reducing ASLR without any real indication that THP is relevant > for the VMA. We need to know if any huge page allocation will be > *attempted* for the VMA or the file. Without a properly aligned address the code will never even attempt allocating a THP. I don't think rounding an address to one that would be properly aligned to map to a THP if possible is all that detrimental to ASLR and without the ability to pick an aligned address it's rather unlikely anyone would ever map anything to a THP unless they explicitly designate an address with MAP_FIXED. If you do object to the slight reduction of the ASLR address space, what alternative would you prefer to see? -- Bill
On Tue, Oct 01, 2019 at 05:21:26AM -0600, William Kucharski wrote: > > > > On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > > > On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: > >> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index cbe7d0619439..670a1780bd2f 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, > >> > >> if (addr) > >> goto out; > >> - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) > >> - goto out; > >> > >> addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); > >> if (addr) > > > > I think you reducing ASLR without any real indication that THP is relevant > > for the VMA. We need to know if any huge page allocation will be > > *attempted* for the VMA or the file. > > Without a properly aligned address the code will never even attempt allocating > a THP. > > I don't think rounding an address to one that would be properly aligned to map > to a THP if possible is all that detrimental to ASLR and without the ability to > pick an aligned address it's rather unlikely anyone would ever map anything to > a THP unless they explicitly designate an address with MAP_FIXED. > > If you do object to the slight reduction of the ASLR address space, what > alternative would you prefer to see? We need to know by the time if THP is allowed for this file/VMA/process/whatever. Meaning that we do not give up ASLR entropy for nothing. For instance, if THP is disabled globally, there is no reason to align the VMA to the THP requirements.
On 10/1/19 5:32 AM, Kirill A. Shutemov wrote: > On Tue, Oct 01, 2019 at 05:21:26AM -0600, William Kucharski wrote: >> >> >>> On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: >>> >>> On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: >>>> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index cbe7d0619439..670a1780bd2f 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, >>>> >>>> if (addr) >>>> goto out; >>>> - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) >>>> - goto out; >>>> >>>> addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); >>>> if (addr) >>> >>> I think you reducing ASLR without any real indication that THP is relevant >>> for the VMA. We need to know if any huge page allocation will be >>> *attempted* for the VMA or the file. >> >> Without a properly aligned address the code will never even attempt allocating >> a THP. >> >> I don't think rounding an address to one that would be properly aligned to map >> to a THP if possible is all that detrimental to ASLR and without the ability to >> pick an aligned address it's rather unlikely anyone would ever map anything to >> a THP unless they explicitly designate an address with MAP_FIXED. >> >> If you do object to the slight reduction of the ASLR address space, what >> alternative would you prefer to see? > > We need to know by the time if THP is allowed for this > file/VMA/process/whatever. Meaning that we do not give up ASLR entropy for > nothing. > > For instance, if THP is disabled globally, there is no reason to align the > VMA to the THP requirements. I understand, but this code is in thp_get_unmapped_area(), which is only called if THP is configured and the VMA can support it. I don't see it in Matthew's patchset, so I'm not sure if it was inadvertently missed in his merge or if he has other ideas for how it would eventually be called, but in my last patch revision the code calling it in do_mmap() looked like this: #ifdef CONFIG_RO_EXEC_FILEMAP_HUGE_FAULT_THP /* * If THP is enabled, it's a read-only executable that is * MAP_PRIVATE mapped, the length is larger than a PMD page * and either it's not a MAP_FIXED mapping or the passed address is * properly aligned for a PMD page, attempt to get an appropriate * address at which to map a PMD-sized THP page, otherwise call the * normal routine. */ if ((prot & PROT_READ) && (prot & PROT_EXEC) && (!(prot & PROT_WRITE)) && (flags & MAP_PRIVATE) && (!(flags & MAP_FIXED)) && len >= HPAGE_PMD_SIZE) { addr = thp_get_unmapped_area(file, addr, len, pgoff, flags); if (addr && (!(addr & ~HPAGE_PMD_MASK))) { /* * If we got a suitable THP mapping address, shut off * VM_MAYWRITE for the region, since it's never what * we would want. */ vm_maywrite = 0; } else addr = get_unmapped_area(file, addr, len, pgoff, flags); } else { #endif So I think that meets your expectations regarding ASLR. -- Bill
On Tue, Oct 01, 2019 at 06:18:28AM -0600, William Kucharski wrote: > > > On 10/1/19 5:32 AM, Kirill A. Shutemov wrote: > > On Tue, Oct 01, 2019 at 05:21:26AM -0600, William Kucharski wrote: > > > > > > > > > > On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > > > > > > > On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: > > > > > > > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > > > > index cbe7d0619439..670a1780bd2f 100644 > > > > > --- a/mm/huge_memory.c > > > > > +++ b/mm/huge_memory.c > > > > > @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, > > > > > > > > > > if (addr) > > > > > goto out; > > > > > - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) > > > > > - goto out; > > > > > > > > > > addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); > > > > > if (addr) > > > > > > > > I think you reducing ASLR without any real indication that THP is relevant > > > > for the VMA. We need to know if any huge page allocation will be > > > > *attempted* for the VMA or the file. > > > > > > Without a properly aligned address the code will never even attempt allocating > > > a THP. > > > > > > I don't think rounding an address to one that would be properly aligned to map > > > to a THP if possible is all that detrimental to ASLR and without the ability to > > > pick an aligned address it's rather unlikely anyone would ever map anything to > > > a THP unless they explicitly designate an address with MAP_FIXED. > > > > > > If you do object to the slight reduction of the ASLR address space, what > > > alternative would you prefer to see? > > > > We need to know by the time if THP is allowed for this > > file/VMA/process/whatever. Meaning that we do not give up ASLR entropy for > > nothing. > > > > For instance, if THP is disabled globally, there is no reason to align the > > VMA to the THP requirements. > > I understand, but this code is in thp_get_unmapped_area(), which is only called > if THP is configured and the VMA can support it. > > I don't see it in Matthew's patchset, so I'm not sure if it was inadvertently > missed in his merge or if he has other ideas for how it would eventually be > called, but in my last patch revision the code calling it in do_mmap() > looked like this: > > #ifdef CONFIG_RO_EXEC_FILEMAP_HUGE_FAULT_THP > /* > * If THP is enabled, it's a read-only executable that is > * MAP_PRIVATE mapped, the length is larger than a PMD page > * and either it's not a MAP_FIXED mapping or the passed address is > * properly aligned for a PMD page, attempt to get an appropriate > * address at which to map a PMD-sized THP page, otherwise call the > * normal routine. > */ > if ((prot & PROT_READ) && (prot & PROT_EXEC) && > (!(prot & PROT_WRITE)) && (flags & MAP_PRIVATE) && > (!(flags & MAP_FIXED)) && len >= HPAGE_PMD_SIZE) { len and MAP_FIXED is already handled by thp_get_unmapped_area(). if (prot & (PROT_READ|PROT_WRITE|PROT_READ) == (PROT_READ|PROT_EXEC) && (flags & MAP_PRIVATE)) { > addr = thp_get_unmapped_area(file, addr, len, pgoff, flags); > > if (addr && (!(addr & ~HPAGE_PMD_MASK))) { This check is broken. For instance, if pgoff is one, (addr & ~HPAGE_PMD_MASK) has to be equal to PAGE_SIZE to have chance to get a huge page in the mapping. > /* > * If we got a suitable THP mapping address, shut off > * VM_MAYWRITE for the region, since it's never what > * we would want. > */ > vm_maywrite = 0; Wouldn't it break uprobe, for instance? > } else > addr = get_unmapped_area(file, addr, len, pgoff, flags); > } else { > #endif > > So I think that meets your expectations regarding ASLR. > > -- Bill
> On Oct 1, 2019, at 8:20 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > On Tue, Oct 01, 2019 at 06:18:28AM -0600, William Kucharski wrote: >> >> >> On 10/1/19 5:32 AM, Kirill A. Shutemov wrote: >>> On Tue, Oct 01, 2019 at 05:21:26AM -0600, William Kucharski wrote: >>>> >>>> >>>>> On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: >>>>> >>>>> On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: >>>>>> >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>>>> index cbe7d0619439..670a1780bd2f 100644 >>>>>> --- a/mm/huge_memory.c >>>>>> +++ b/mm/huge_memory.c >>>>>> @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, >>>>>> >>>>>> if (addr) >>>>>> goto out; >>>>>> - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) >>>>>> - goto out; >>>>>> >>>>>> addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); >>>>>> if (addr) >>>>> >>>>> I think you reducing ASLR without any real indication that THP is relevant >>>>> for the VMA. We need to know if any huge page allocation will be >>>>> *attempted* for the VMA or the file. >>>> >>>> Without a properly aligned address the code will never even attempt allocating >>>> a THP. >>>> >>>> I don't think rounding an address to one that would be properly aligned to map >>>> to a THP if possible is all that detrimental to ASLR and without the ability to >>>> pick an aligned address it's rather unlikely anyone would ever map anything to >>>> a THP unless they explicitly designate an address with MAP_FIXED. >>>> >>>> If you do object to the slight reduction of the ASLR address space, what >>>> alternative would you prefer to see? >>> >>> We need to know by the time if THP is allowed for this >>> file/VMA/process/whatever. Meaning that we do not give up ASLR entropy for >>> nothing. >>> >>> For instance, if THP is disabled globally, there is no reason to align the >>> VMA to the THP requirements. >> >> I understand, but this code is in thp_get_unmapped_area(), which is only called >> if THP is configured and the VMA can support it. >> >> I don't see it in Matthew's patchset, so I'm not sure if it was inadvertently >> missed in his merge or if he has other ideas for how it would eventually be >> called, but in my last patch revision the code calling it in do_mmap() >> looked like this: >> >> #ifdef CONFIG_RO_EXEC_FILEMAP_HUGE_FAULT_THP >> /* >> * If THP is enabled, it's a read-only executable that is >> * MAP_PRIVATE mapped, the length is larger than a PMD page >> * and either it's not a MAP_FIXED mapping or the passed address is >> * properly aligned for a PMD page, attempt to get an appropriate >> * address at which to map a PMD-sized THP page, otherwise call the >> * normal routine. >> */ >> if ((prot & PROT_READ) && (prot & PROT_EXEC) && >> (!(prot & PROT_WRITE)) && (flags & MAP_PRIVATE) && >> (!(flags & MAP_FIXED)) && len >= HPAGE_PMD_SIZE) { > > len and MAP_FIXED is already handled by thp_get_unmapped_area(). > > if (prot & (PROT_READ|PROT_WRITE|PROT_READ) == (PROT_READ|PROT_EXEC) && > (flags & MAP_PRIVATE)) { It is, but I wanted to avoid even calling it if conditions weren't right. Checking twice is non-optimal but I didn't want to alter the existing use of the routine for anon THP. > > >> addr = thp_get_unmapped_area(file, addr, len, pgoff, flags); >> >> if (addr && (!(addr & ~HPAGE_PMD_MASK))) { > > This check is broken. > > For instance, if pgoff is one, (addr & ~HPAGE_PMD_MASK) has to be equal to > PAGE_SIZE to have chance to get a huge page in the mapping. > If the address isn't PMD-aligned, we will never be able to map it with a THP anyway. The current code is designed to only map a THP if the VMA allows for it and it can map the entire THP starting at an aligned address. You can't map a THP at the PMD level at an address that isn't PMD aligned. Perhaps I'm missing a use case here. >> /* >> * If we got a suitable THP mapping address, shut off >> * VM_MAYWRITE for the region, since it's never what >> * we would want. >> */ >> vm_maywrite = 0; > > Wouldn't it break uprobe, for instance? I'm not sure; does uprobe allow COW to insert the probe even for mappings explicitly marked read-only? Thanks, Bill
On Tue, Oct 01, 2019 at 10:08:30AM -0600, William Kucharski wrote: > > > > On Oct 1, 2019, at 8:20 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > > > On Tue, Oct 01, 2019 at 06:18:28AM -0600, William Kucharski wrote: > >> > >> > >> On 10/1/19 5:32 AM, Kirill A. Shutemov wrote: > >>> On Tue, Oct 01, 2019 at 05:21:26AM -0600, William Kucharski wrote: > >>>> > >>>> > >>>>> On Oct 1, 2019, at 4:45 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > >>>>> > >>>>> On Tue, Sep 24, 2019 at 05:52:13PM -0700, Matthew Wilcox wrote: > >>>>>> > >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >>>>>> index cbe7d0619439..670a1780bd2f 100644 > >>>>>> --- a/mm/huge_memory.c > >>>>>> +++ b/mm/huge_memory.c > >>>>>> @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, > >>>>>> > >>>>>> if (addr) > >>>>>> goto out; > >>>>>> - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) > >>>>>> - goto out; > >>>>>> > >>>>>> addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); > >>>>>> if (addr) > >>>>> > >>>>> I think you reducing ASLR without any real indication that THP is relevant > >>>>> for the VMA. We need to know if any huge page allocation will be > >>>>> *attempted* for the VMA or the file. > >>>> > >>>> Without a properly aligned address the code will never even attempt allocating > >>>> a THP. > >>>> > >>>> I don't think rounding an address to one that would be properly aligned to map > >>>> to a THP if possible is all that detrimental to ASLR and without the ability to > >>>> pick an aligned address it's rather unlikely anyone would ever map anything to > >>>> a THP unless they explicitly designate an address with MAP_FIXED. > >>>> > >>>> If you do object to the slight reduction of the ASLR address space, what > >>>> alternative would you prefer to see? > >>> > >>> We need to know by the time if THP is allowed for this > >>> file/VMA/process/whatever. Meaning that we do not give up ASLR entropy for > >>> nothing. > >>> > >>> For instance, if THP is disabled globally, there is no reason to align the > >>> VMA to the THP requirements. > >> > >> I understand, but this code is in thp_get_unmapped_area(), which is only called > >> if THP is configured and the VMA can support it. > >> > >> I don't see it in Matthew's patchset, so I'm not sure if it was inadvertently > >> missed in his merge or if he has other ideas for how it would eventually be > >> called, but in my last patch revision the code calling it in do_mmap() > >> looked like this: > >> > >> #ifdef CONFIG_RO_EXEC_FILEMAP_HUGE_FAULT_THP > >> /* > >> * If THP is enabled, it's a read-only executable that is > >> * MAP_PRIVATE mapped, the length is larger than a PMD page > >> * and either it's not a MAP_FIXED mapping or the passed address is > >> * properly aligned for a PMD page, attempt to get an appropriate > >> * address at which to map a PMD-sized THP page, otherwise call the > >> * normal routine. > >> */ > >> if ((prot & PROT_READ) && (prot & PROT_EXEC) && > >> (!(prot & PROT_WRITE)) && (flags & MAP_PRIVATE) && > >> (!(flags & MAP_FIXED)) && len >= HPAGE_PMD_SIZE) { > > > > len and MAP_FIXED is already handled by thp_get_unmapped_area(). > > > > if (prot & (PROT_READ|PROT_WRITE|PROT_READ) == (PROT_READ|PROT_EXEC) && > > (flags & MAP_PRIVATE)) { > > It is, but I wanted to avoid even calling it if conditions weren't right. > > Checking twice is non-optimal but I didn't want to alter the existing use of > the routine for anon THP. It's not used by anon THP. It used for DAX. > > > > > >> addr = thp_get_unmapped_area(file, addr, len, pgoff, flags); > >> > >> if (addr && (!(addr & ~HPAGE_PMD_MASK))) { > > > > This check is broken. > > > > For instance, if pgoff is one, (addr & ~HPAGE_PMD_MASK) has to be equal to > > PAGE_SIZE to have chance to get a huge page in the mapping. > > > > If the address isn't PMD-aligned, we will never be able to map it with a THP > anyway. The opposite is true. I tried to explain it few times, but let's try again. If the address here is PMD-aligned, it will get a mismatch with page cache alignment. Consider the case with 2 huge pages in page cache, starting at the beginning of the file. The key to understanding: huge pages always aligned naturally in page cache. Page cache lives longer than any mapping of the file. If user calls mmap(.pgoff = 1) and it returns PMD-aligned address, we will never have a huge page in the mapping. At the PMD-aligned address you will get the second 4k of the first huge page and you will have to map it with PTE. At the second PMD-aligned address of the mapping, the situation will repeat for the second huge page, again misaligned, PTE-mapped second subpage. The solution here is to return address aligned to PMD_SIZE + PAGE_SIZE, if user asked for mmap(.pgoff = 1). The user will not get first huge page mapped with PMD, because mapping truncates it from the beginning per user request. But the second (and any following huge page) will land on the right alignment and can be mapped with PMD. Does it make sense? > >> /* > >> * If we got a suitable THP mapping address, shut off > >> * VM_MAYWRITE for the region, since it's never what > >> * we would want. > >> */ > >> vm_maywrite = 0; > > > > Wouldn't it break uprobe, for instance? > > I'm not sure; does uprobe allow COW to insert the probe even for mappings > explicitly marked read-only? Yes. See FOLL_FORCE usage.
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index cbe7d0619439..670a1780bd2f 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -563,8 +563,6 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr, if (addr) goto out; - if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD)) - goto out; addr = __thp_get_unmapped_area(filp, len, off, flags, PMD_SIZE); if (addr)