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: <8cc5b94c-f861-4ca1-b339-704140ad9255@redhat.com>
Date: Thu, 1 Aug 2024 18:24:53 +0200
From: David Hildenbrand <david@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 James Houghton <jthoughton@...gle.com>, stable@...r.kernel.org,
 Oscar Salvador <osalvador@...e.de>, Muchun Song <muchun.song@...ux.dev>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>
Subject: Re: [PATCH v3] mm/hugetlb: fix hugetlb vs. core-mm PT locking

On 01.08.24 18:07, Peter Xu wrote:
> On Thu, Aug 01, 2024 at 05:35:20PM +0200, David Hildenbrand wrote:
>> Hi Peter,
> 
> [...]
> 
>>>> +	else if (size >= PUD_SIZE)
>>>> +		return pud_lockptr(mm, (pud_t *) pte);
>>>> +	else if (size >= PMD_SIZE || IS_ENABLED(CONFIG_HIGHPTE))
>>>
>>> I thought this HIGHPTE can also be dropped? Because in HIGHPTE it should
>>> never have lower-than-PMD huge pages or we're in trouble.  That's why I
>>> kept one WARN_ON() in my pesudo code but only before trying to take the pte
>>> lockptr.
>>
>> Then the compiler won't optimize out the ptep_lockptr() call and we'll run
>> into a build error. And I think the HIGHPTE builderror serves good purpose.
>>
>> In file included from <command-line>:
>> In function 'ptep_lockptr',
>>      inlined from 'huge_pte_lockptr' at ./include/linux/hugetlb.h:974:9,
>>      inlined from 'huge_pte_lock' at ./include/linux/hugetlb.h:1248:8,
>>      inlined from 'pagemap_scan_hugetlb_entry' at fs/proc/task_mmu.c:2581:8:
>> ././include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_256' declared with attribute error: BUILD_BUG_ON failed: IS_ENABLED(CONFIG_HIGHPTE)
>>    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>        |                                             ^
>> ././include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert'
>>    491 |                         prefix ## suffix();                             \
>>        |                         ^~~~~~
>> ././include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
>>    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>>        |         ^~~~~~~~~~~~~~~~~~~
>> ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
>>     39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>>        |                                     ^~~~~~~~~~~~~~~~~~
>> ./include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>>     50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>>        |         ^~~~~~~~~~~~~~~~
>> ./include/linux/mm.h:2874:9: note: in expansion of macro 'BUILD_BUG_ON'
>>   2874 |         BUILD_BUG_ON(IS_ENABLED(CONFIG_HIGHPTE));
> 
> Ahh.. this is in "ifdef USE_SPLIT_PTE_PTLOCKS" section.  I'm thinking maybe
> we should drop this BUILD_BUG_ON - it says "HIGHPTE shouldn't co-exist with
> USE_SPLIT_PTE_PTLOCKS", but I think it can?
> 
> Said that, I think I can also understand your point, where you see
> ptep_lockptr() a hugetlb-only function, in that case the BUILD_BUG_ON would
> make sense in hugetlb world.
> 
> So.. per my previous nitpick suggestion, IIUC we'll need to drop this
> BUILD_BUG_ON, just to say "USE_SPLIT_PTE_PTLOCKS can work with HIGHPTE" and
> perhaps slightly more readable; we'll rely on the WARN_ON to guard HIGHPTE
> won't use pte lock.

I really don't want to  drop the BUILD_BUG_ON. The function cannot 
possibly work with HIGHPTE, especially once used in other context by 
accident.

So I'll leave it like that. Feel free to optimize the hugetlb code 
further once the fix has landed (e.g., really optimize it out if we 
cannot possibly have such hugetlb sizes).

Thanks!

-- 
Cheers,

David / dhildenb


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ