+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@xxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxxxx> > Closes: https://lore.kernel.org/Z6rgiVp7221r4JZ5@ly-workstation > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx> > --- > 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<----
