All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fs/proc/task_mmu: move mmu notification mechanism inside mm lock
@ 2024-01-09 11:24 Muhammad Usama Anjum
  2024-01-09 16:28 ` Sean Christopherson
  0 siblings, 1 reply; 3+ messages in thread
From: Muhammad Usama Anjum @ 2024-01-09 11:24 UTC (permalink / raw)
  To: Andrew Morton, David Hildenbrand, Muhammad Usama Anjum,
	Andrei Vagin, Peter Xu, Hugh Dickins, Suren Baghdasaryan,
	Ryan Roberts, Kefeng Wang, Liam R. Howlett,
	Michał Mirosław, Stephen Rothwell, Arnd Bergmann
  Cc: kernel, syzbot+81227d2bd69e9dedb802, Sean Christopherson, stable,
	linux-kernel, linux-fsdevel

Move mmu notification mechanism inside mm lock to prevent race condition
in other components which depend on it. The notifier will invalidate
memory range. Depending upon the number of iterations, different memory
ranges would be invalidated.

The following warning would be removed by this patch:
WARNING: CPU: 0 PID: 5067 at arch/x86/kvm/../../../virt/kvm/kvm_main.c:734 kvm_mmu_notifier_change_pte+0x860/0x960 arch/x86/kvm/../../../virt/kvm/kvm_main.c:734

There is no behavioural and performance change with this patch when
there is no component registered with the mmu notifier.

Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
Reported-by: syzbot+81227d2bd69e9dedb802@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/000000000000f6d051060c6785bc@google.com/
Cc: Sean Christopherson <seanjc@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 fs/proc/task_mmu.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 62b16f42d5d2..56c2e7357494 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -2448,13 +2448,6 @@ static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
 	if (ret)
 		return ret;
 
-	/* Protection change for the range is going to happen. */
-	if (p.arg.flags & PM_SCAN_WP_MATCHING) {
-		mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
-					mm, p.arg.start, p.arg.end);
-		mmu_notifier_invalidate_range_start(&range);
-	}
-
 	for (walk_start = p.arg.start; walk_start < p.arg.end;
 			walk_start = p.arg.walk_end) {
 		long n_out;
@@ -2467,8 +2460,20 @@ static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
 		ret = mmap_read_lock_killable(mm);
 		if (ret)
 			break;
+
+		/* Protection change for the range is going to happen. */
+		if (p.arg.flags & PM_SCAN_WP_MATCHING) {
+			mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
+						mm, walk_start, p.arg.end);
+			mmu_notifier_invalidate_range_start(&range);
+		}
+
 		ret = walk_page_range(mm, walk_start, p.arg.end,
 				      &pagemap_scan_ops, &p);
+
+		if (p.arg.flags & PM_SCAN_WP_MATCHING)
+			mmu_notifier_invalidate_range_end(&range);
+
 		mmap_read_unlock(mm);
 
 		n_out = pagemap_scan_flush_buffer(&p);
@@ -2494,9 +2499,6 @@ static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
 	if (pagemap_scan_writeback_args(&p.arg, uarg))
 		ret = -EFAULT;
 
-	if (p.arg.flags & PM_SCAN_WP_MATCHING)
-		mmu_notifier_invalidate_range_end(&range);
-
 	kfree(p.vec_buf);
 	return ret;
 }
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] fs/proc/task_mmu: move mmu notification mechanism inside mm lock
  2024-01-09 11:24 [PATCH] fs/proc/task_mmu: move mmu notification mechanism inside mm lock Muhammad Usama Anjum
@ 2024-01-09 16:28 ` Sean Christopherson
  2024-01-10  4:43   ` Andrew Morton
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2024-01-09 16:28 UTC (permalink / raw)
  To: Muhammad Usama Anjum
  Cc: Andrew Morton, David Hildenbrand, Andrei Vagin, Peter Xu,
	Hugh Dickins, Suren Baghdasaryan, Ryan Roberts, Kefeng Wang,
	Liam R. Howlett, Michał Mirosław, Stephen Rothwell,
	Arnd Bergmann, kernel, syzbot+81227d2bd69e9dedb802, stable,
	linux-kernel, linux-fsdevel

On Tue, Jan 09, 2024, Muhammad Usama Anjum wrote:
> Move mmu notification mechanism inside mm lock to prevent race condition
> in other components which depend on it. The notifier will invalidate
> memory range. Depending upon the number of iterations, different memory
> ranges would be invalidated.
> 
> The following warning would be removed by this patch:
> WARNING: CPU: 0 PID: 5067 at arch/x86/kvm/../../../virt/kvm/kvm_main.c:734 kvm_mmu_notifier_change_pte+0x860/0x960 arch/x86/kvm/../../../virt/kvm/kvm_main.c:734
> 
> There is no behavioural and performance change with this patch when
> there is no component registered with the mmu notifier.
> 
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+81227d2bd69e9dedb802@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/000000000000f6d051060c6785bc@google.com/
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  fs/proc/task_mmu.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 62b16f42d5d2..56c2e7357494 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -2448,13 +2448,6 @@ static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
>  	if (ret)
>  		return ret;
>  
> -	/* Protection change for the range is going to happen. */
> -	if (p.arg.flags & PM_SCAN_WP_MATCHING) {
> -		mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
> -					mm, p.arg.start, p.arg.end);
> -		mmu_notifier_invalidate_range_start(&range);
> -	}
> -
>  	for (walk_start = p.arg.start; walk_start < p.arg.end;
>  			walk_start = p.arg.walk_end) {
>  		long n_out;

Nit, might be worth moving

		struct mmu_notifier_range range;

inside the loop to guard against stale usage, but that's definitely optional.

Reviewed-by: Sean Christopherson <seanjc@google.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fs/proc/task_mmu: move mmu notification mechanism inside mm lock
  2024-01-09 16:28 ` Sean Christopherson
@ 2024-01-10  4:43   ` Andrew Morton
  0 siblings, 0 replies; 3+ messages in thread
From: Andrew Morton @ 2024-01-10  4:43 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Muhammad Usama Anjum, David Hildenbrand, Andrei Vagin, Peter Xu,
	Hugh Dickins, Suren Baghdasaryan, Ryan Roberts, Kefeng Wang,
	Liam R. Howlett,  Michał Mirosław ,
	Stephen Rothwell, Arnd Bergmann, kernel,
	syzbot+81227d2bd69e9dedb802, stable, linux-kernel, linux-fsdevel

On Tue, 9 Jan 2024 08:28:06 -0800 Sean Christopherson <seanjc@google.com> wrote:

> > -	/* Protection change for the range is going to happen. */
> > -	if (p.arg.flags & PM_SCAN_WP_MATCHING) {
> > -		mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0,
> > -					mm, p.arg.start, p.arg.end);
> > -		mmu_notifier_invalidate_range_start(&range);
> > -	}
> > -
> >  	for (walk_start = p.arg.start; walk_start < p.arg.end;
> >  			walk_start = p.arg.walk_end) {
> >  		long n_out;
> 
> Nit, might be worth moving
> 
> 		struct mmu_notifier_range range;
> 
> inside the loop to guard against stale usage, but that's definitely optional.

Yes, I think that's nicer.

--- a/fs/proc/task_mmu.c~fs-proc-task_mmu-move-mmu-notification-mechanism-inside-mm-lock-fix
+++ a/fs/proc/task_mmu.c
@@ -2432,7 +2432,6 @@ static long pagemap_scan_flush_buffer(st
 
 static long do_pagemap_scan(struct mm_struct *mm, unsigned long uarg)
 {
-	struct mmu_notifier_range range;
 	struct pagemap_scan_private p = {0};
 	unsigned long walk_start;
 	size_t n_ranges_out = 0;
@@ -2450,6 +2449,7 @@ static long do_pagemap_scan(struct mm_st
 
 	for (walk_start = p.arg.start; walk_start < p.arg.end;
 			walk_start = p.arg.walk_end) {
+		struct mmu_notifier_range range;
 		long n_out;
 
 		if (fatal_signal_pending(current)) {
_


I'm surprised this code doesn't generate a might-be-used-uninitialized
warning.  I guess gcc got smarter.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-01-10  4:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-09 11:24 [PATCH] fs/proc/task_mmu: move mmu notification mechanism inside mm lock Muhammad Usama Anjum
2024-01-09 16:28 ` Sean Christopherson
2024-01-10  4:43   ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.