| 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
| ||
|
Message-ID: <2f448f7b-1da7-4099-aa9e-0179d47fde40@lucifer.local>
Date: Tue, 11 Feb 2025 10:51:41 +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()
+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@...ux.intel.com>
> Closes: https://lore.kernel.org/Z6rgiVp7221r4JZ5@ly-workstation
> Signed-off-by: SeongJae Park <sj@...nel.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@...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>
---
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
Powered by blists - more mailing lists