linux-kernel - Re: [PATCH mm-unstable] mm/madvise: handle MADV_{HWPOISON,SOFT_OFFLINE} from madvise

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: <a822501c-37ac-40a1-8b1b-4765ad5fd86d@lucifer.local>
Date: Wed, 12 Feb 2025 11:35:52 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <howlett@...il.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Shakeel Butt <shakeel.butt@...ux.dev>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, "Lai, Yi" <yi1.lai@...ux.intel.com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH mm-unstable] mm/madvise: handle
 MADV_{HWPOISON,SOFT_OFFLINE} from madvise_unlock()

On Tue, Feb 11, 2025 at 10:20:53AM -0800, SeongJae Park wrote:
> On Tue, 11 Feb 2025 10:51:41 +0000 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
>
> > +cc Naresh, Arnd for another reports/discussion of the same issue [0] while
> > lore/lei is broken.
> >
> > Hi,
> >
> > Lore breaking means I missed this :) thankfully you cc'd me (_this_ is why I am
> > so adament about people following get_maintainer.pl procedure btw) so I was able
> > to now notice + reply :)
> >
> > This is totally my bad for missing this on review, so mea culpa.
>
> No worry, your reviews are always very helpful!

Very kind of you to say :>)

>
> >
> > [0]:https://lwn.net/ml/linux-mm/CA+G9fYt5QwJ4_F8fJj7jx9_0Le9kOVSeG38ox9qnKqwsrDdvHQ@mail.gmail.com/
> >
> > On Mon, Feb 10, 2025 at 10:32:01PM -0800, SeongJae Park wrote:
> > > madvise_lock() does nothing for MADV_HWPOSION and MADV_SOFT_OFFLINE
> > > behavior, but madvise_unlock() does mmap_lock unlocking regardless of
> > > the behavior.
> [...]
> > >  mm/madvise.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index b5ef8e03d8b0..b8969457f3ef 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1577,7 +1577,6 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > >
> > >  static int madvise_lock(struct mm_struct *mm, int behavior)
> > >  {
> > > -
> > >  #ifdef CONFIG_MEMORY_FAILURE
> > >  	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
> > >  		return 0;
> > > @@ -1595,6 +1594,11 @@ static int madvise_lock(struct mm_struct *mm, int behavior)
> > >
> > >  static void madvise_unlock(struct mm_struct *mm, int behavior)
> > >  {
> > > +#ifdef CONFIG_MEMORY_FAILURE
> > > +	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
> > > +		return;
> > > +#endif
> >
> > I agree this fixes the issue but this is horrible. let's abstract this please
> > rather than doing the same crap that already existed, only now twice.
>
> I agree abstracting this is a better idea.

Thanks!

>
> >
> > > +
> > >  	if (madvise_need_mmap_write(behavior))
> > >  		mmap_write_unlock(mm);
> > >  	else
> > >
> > > base-commit: 8bf30f9d23eb5040d37e6e712789cee8e71e1577
> > > --
> > > 2.39.5
> >
> > I attach a fix-patch concept for something I think that'd be nicer, do with
> > it what thy wilt! :P sorry I don't mean to be 'one of those' maintainers
> > who copy/pastes code + demands somebody do it (by no means do I do so), but
> > since this is so small I feel it's kind of quicker for me to do it this
> > way.
> >
> > Obviously take it or leave it/adapt it/etc. This is compile-tested only...
>
> I further ran the repro program and confirmed this fixes the issue :)

Great, perfect!

>
> >
> > ----8<----
> > From 9fce3e47bf0fff2a2291be66002af346cdbca665 Mon Sep 17 00:00:00 2001
> > From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > Date: Tue, 11 Feb 2025 10:44:26 +0000
> > Subject: [PATCH] mm/madvise: fix madvise_[un]lock() issue
> >
> > We are asymmetric in our locking/unlocking in the case of memory failure
> > madvise() behaviour options, correct this and abstract the memory failure
> > check.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Reviewed-by: SeongJae Park <sj@...nel.org>
> Tested-by: SeongJae Park <sj@...nel.org>

Thanks :)

I accidentally seem to have included some whitespace errors which Andrew
kindly fixed up :>) so apologies for that!

>
>
> Thanks,
> SJ
>
> [...]

Cheers, Lorenzo