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] [day] [month] [year] [list]

Message-ID: <Z1MPlgsli-eA4o7z@bender.morinfr.org>
Date: Fri, 6 Dec 2024 15:52:06 +0100
From: Guillaume Morin <guillaume@...infr.org>
To: David Hildenbrand <david@...hat.com>
Cc: Heiko Carstens <hca@...ux.ibm.com>,
	Guillaume Morin <guillaume@...infr.org>,
	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 Dec 10:29, David Hildenbrand wrote:
>
> 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:
> > > > 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.

Thanks all. I dropped the check and the dummy definition I had to add
for i386 in v4 [1]

[1] https://lore.kernel.org/linux-mm/Z1MO5slZh8uWl8LH@bender.morinfr.org/T/#u

-- 
Guillaume Morin <guillaume@...infr.org>