linux-kernel - Re: [PATCH v3] mm/hugetlb: support FOLL_FORCE|FOLL

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: <c43a3149-c84b-448b-be80-1e026740911c@redhat.com>
Date: Fri, 6 Dec 2024 10:29:31 +0100
From: David Hildenbrand <david@...hat.com>
To: Heiko Carstens <hca@...ux.ibm.com>,
 Guillaume Morin <guillaume@...infr.org>
Cc: Nathan Chancellor <nathan@...nel.org>, Vasily Gorbik <gor@...ux.ibm.com>,
 Alexander Gordeev <agordeev@...ux.ibm.com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, Muchun Song <muchun.song@...ux.dev>,
 Andrew Morton <akpm@...ux-foundation.org>, Peter Xu <peterx@...hat.com>,
 Eric Hagberg <ehagberg@...estreet.com>, linux-s390@...r.kernel.org
Subject: Re: [PATCH v3] mm/hugetlb: support FOLL_FORCE|FOLL_WRITE

On 06.12.24 10:24, Heiko Carstens wrote:
> On Fri, Dec 06, 2024 at 06:27:09AM +0100, Guillaume Morin wrote:
>> On 05 Dec 21:50, Nathan Chancellor wrote:
>>>>   #ifdef CONFIG_PGTABLE_HAS_HUGE_LEAVES
>>>> +/* FOLL_FORCE can write to even unwritable PUDs in COW mappings. */
>>>> +static inline bool can_follow_write_pud(pud_t pud, struct page *page,
>>>> +					struct vm_area_struct *vma,
>>>> +					unsigned int flags)
>>>> +{
>>>> +	/* If the pud is writable, we can write to the page. */
>>>> +	if (pud_write(pud))
>>>> +		return true;
>>>> +
>>>> +	if (!can_follow_write_common(page, vma, flags))
>>>> +		return false;
>>>> +
>>>> +	/* ... and a write-fault isn't required for other reasons. */
>>>> +	return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud);
>>>
>>> This looks to be one of the first uses of pud_soft_dirty() in a generic
>>> part of the tree from what I can tell, which shows that s390 is lacking
>>> it despite setting CONFIG_HAVE_ARCH_SOFT_DIRTY:
>>>
>>>    $ make -skj"$(nproc)" ARCH=s390 CROSS_COMPILE=s390-linux- mrproper defconfig mm/gup.o
>>>    mm/gup.c: In function 'can_follow_write_pud':
>>>    mm/gup.c:665:48: error: implicit declaration of function 'pud_soft_dirty'; did you mean 'pmd_soft_dirty'? [-Wimplicit-function-declaration]
>>>      665 |         return !vma_soft_dirty_enabled(vma) || pud_soft_dirty(pud);
>>>          |                                                ^~~~~~~~~~~~~~
>>>          |                                                pmd_soft_dirty
>>>
>>> Is this expected?
>>
>> Yikes! It does look like an oversight in the s390 code since as you said
>> it has CONFIG_HAVE_ARCH_SOFT_DIRTY and pud_mkdirty seems to be setting
>> _REGION3_ENTRY_SOFT_DIRTY. But I'll let the s390 folks opine.
>>
>> I don't mind dropping the pud part of the change (even if that's a bit
>> of a shame) if it's causing too many issues.
> 
> It would be quite easy to add pud_soft_dirty() etc. helper functions
> for s390, but I think that would be the wrong answer to this problem.
> 
> s390 implements pud_mkdirty(), but it is only used in the context of
> HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD, which s390 doesn't support. So
> this function should probably be removed from s390's pgtable.h.
> 
> Similar the pud_soft_dirty() and friends helper functions should only
> be implemented if common code support for soft dirty would exist,
> which is currently not the case. Otherwise similar fallbacks like for
> pmd_soft_dirty() (-> include/linux/pgtable.h) would also need to be
> implemented.
> 
> So IMHO the right fix (at this time) seems to be to remove the above
> pud part of your patch, and in addition we should probably also drop
> the partially implemented pud level soft dirty bits in s390 code,
> since that is dead code and might cause even more confusion in future.
> 
> Does that make sense?

As hugetlb does not support softdirty, and PUDs are currently only 
possible (weird DAX thing put aside) with hugetlb, it makes sense to 
drop the pud softdirty thingy.

-- 
Cheers,

David / dhildenb