diff mbox series

[mm-unstable] mm/madvise: handle MADV_{HWPOISON,SOFT_OFFLINE} from madvise_unlock()

Message ID 20250211063201.5106-1-sj@kernel.org (mailing list archive)
State New
Headers show
Series [mm-unstable] mm/madvise: handle MADV_{HWPOISON,SOFT_OFFLINE} from madvise_unlock() | expand

Commit Message

SeongJae Park Feb. 11, 2025, 6:32 a.m. UTC 
madvise_lock() does nothing for MADV_HWPOSION and MADV_SOFT_OFFLINE
behavior, but madvise_unlock() does mmap_lock unlocking regardless of
the behavior.  Commit 948a0a9ea070 ("mm/madvise: split out mmap locking
operations for madvise()") in mm-unstable, which introduced the wrong
function didn't cause a real problem because do_madvise() was not
calling madvise_unlock() for the behavior.

Later, commit f19c9d7b57cf ("mm/madvise: split out madvise() behavior
execution") in mm-unstable made do_madvise() to call madvise_unlock()
even for the two behaviors.  As a result, the kernel tries to unlock
unlocked mmap_lock.

Fix the issue by handling the two behaviors in madvise_unlock().  For
the two behaviors, do nothing but just return.  Also remove an
unnecessary blank line in madvise_lock().

Technically speaking this patch fixes commit f19c9d7b57cf ("mm/madvise:
split out madvise() behavior execution").  But since the broken commit
is not in the mainline yet, squashing this fix into commit 948a0a9ea070
("mm/madvise: split out mmap locking operations for madvise()") would
make more sense, so adding Fixes: tag with it.

Fixes: 948a0a9ea070 ("mm/madvise: split out mmap locking operations for madvise()") # mm-unstable
Reported-by: "Lai, Yi" <yi1.lai@linux.intel.com>
Closes: https://lore.kernel.org/Z6rgiVp7221r4JZ5@ly-workstation
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/madvise.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)


base-commit: 8bf30f9d23eb5040d37e6e712789cee8e71e1577

Comments

Lorenzo Stoakes Feb. 11, 2025, 10:51 a.m. UTC | #1 
+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.

[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.  Commit 948a0a9ea070 ("mm/madvise: split out mmap locking
> operations for madvise()") in mm-unstable, which introduced the wrong
> function didn't cause a real problem because do_madvise() was not
> calling madvise_unlock() for the behavior.
>
> Later, commit f19c9d7b57cf ("mm/madvise: split out madvise() behavior
> execution") in mm-unstable made do_madvise() to call madvise_unlock()
> even for the two behaviors.  As a result, the kernel tries to unlock
> unlocked mmap_lock.
>
> Fix the issue by handling the two behaviors in madvise_unlock().  For
> the two behaviors, do nothing but just return.  Also remove an
> unnecessary blank line in madvise_lock().
>
> Technically speaking this patch fixes commit f19c9d7b57cf ("mm/madvise:
> split out madvise() behavior execution").  But since the broken commit
> is not in the mainline yet, squashing this fix into commit 948a0a9ea070
> ("mm/madvise: split out mmap locking operations for madvise()") would
> make more sense, so adding Fixes: tag with it.
>
> Fixes: 948a0a9ea070 ("mm/madvise: split out mmap locking operations for madvise()") # mm-unstable
> Reported-by: "Lai, Yi" <yi1.lai@linux.intel.com>
> Closes: https://lore.kernel.org/Z6rgiVp7221r4JZ5@ly-workstation
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
>  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.

> +
>  	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...

----8<----
From 9fce3e47bf0fff2a2291be66002af346cdbca665 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.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@oracle.com>
---
 mm/madvise.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index b5ef8e03d8b0..1a7af59c3aa9 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1575,14 +1575,29 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
 }
 #endif /* CONFIG_ANON_VMA_NAME */

-static int madvise_lock(struct mm_struct *mm, int behavior)
-{
-
 #ifdef CONFIG_MEMORY_FAILURE
-	if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
-		return 0;
+static bool is_memory_failure(int behavior)
+{
+	switch (behavior) {
+	case MADV_HWPOISON:
+	case MADV_SOFT_OFFLINE:
+		return true;
+	default:
+		return false;
+	}
+}
+#else
+static bool is_memory_failure(int behavior)
+{
+	return false;
+}
 #endif

+static int madvise_lock(struct mm_struct *mm, int behavior)
+{
+	if (is_memory_failure(behavior))
+	    return 0;
+
 	if (madvise_need_mmap_write(behavior)) {
 		if (mmap_write_lock_killable(mm))
 			return -EINTR;
@@ -1590,11 +1605,13 @@ static int madvise_lock(struct mm_struct *mm, int behavior)
 		mmap_read_lock(mm);
 	}
 	return 0;
-
 }

 static void madvise_unlock(struct mm_struct *mm, int behavior)
 {
+	if (is_memory_failure(behavior))
+	    return;
+
 	if (madvise_need_mmap_write(behavior))
 		mmap_write_unlock(mm);
 	else
--
2.48.1
SeongJae Park Feb. 11, 2025, 6:20 p.m. UTC | #2 
On Tue, 11 Feb 2025 10:51:41 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.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!

> 
> [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.

> 
> > +
> >  	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 :)

> 
> ----8<----
> From 9fce3e47bf0fff2a2291be66002af346cdbca665 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.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@oracle.com>

Reviewed-by: SeongJae Park <sj@kernel.org>
Tested-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]
Lorenzo Stoakes Feb. 12, 2025, 11:35 a.m. UTC | #3 
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@oracle.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@oracle.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@oracle.com>
>
> Reviewed-by: SeongJae Park <sj@kernel.org>
> Tested-by: SeongJae Park <sj@kernel.org>

Thanks :)

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

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

Cheers, Lorenzo
diff mbox series

Patch

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
+
 	if (madvise_need_mmap_write(behavior))
 		mmap_write_unlock(mm);
 	else