Messages in this thread | | | Date | Tue, 14 Nov 2017 22:21:13 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCHv2 1/2] x86/mm: Do not allow non-MAP_FIXED mapping across DEFAULT_MAP_WINDOW border |
| |
On Tue, Nov 14, 2017 at 05:01:50PM +0100, Thomas Gleixner wrote: > On Tue, 14 Nov 2017, Kirill A. Shutemov wrote: > > --- a/arch/x86/mm/hugetlbpage.c > > +++ b/arch/x86/mm/hugetlbpage.c > > @@ -166,11 +166,20 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr, > > > > if (addr) { > > addr = ALIGN(addr, huge_page_size(h)); > > + if (TASK_SIZE - len >= addr) > > + goto get_unmapped_area; > > That's wrong. You got it right in arch_get_unmapped_area_topdown() ...
Ouch.
Please ignore selftest patch. I'll rework it to cover hugetlb.
> > + > > + /* See a comment in arch_get_unmapped_area_topdown */ > > This is lame, really. > > > + if ((addr > DEFAULT_MAP_WINDOW) != > > + (addr + len > DEFAULT_MAP_WINDOW)) > > + goto get_unmapped_area; > > Instead of duplicating that horrible formatted condition and adding this > lousy comment why can't you just put all of it (including the TASK_SIZE > check) into a proper validation function and put the comment there? > > The fixed up variant of your patch below does that. > > Aside of that please spend a bit more time on describing things precisely > at the technical and factual level next time. I fixed that up (once more) > both in the comment and the changelog. > > Please double check.
Works fine.
> +bool mmap_address_hint_valid(unsigned long addr, unsigned long len) > +{ > + if (TASK_SIZE - len < addr) > + return false; > +#if CONFIG_PGTABLE_LEVELS >= 5 > + return (addr > DEFAULT_MAP_WINDOW) == (addr + len > DEFAULT_MAP_WINDOW);
Is it micro optimization? I don't feel it necessary. It's not that hot codepath to care about few cycles. (And one more place to care about for boot-time switching.)
If you think it's needed, maybe IS_ENABLED() instead?
> +#else > + return true; > +#endif > +}
-- Kirill A. Shutemov
| |