Re: [linus:master] [gup] a425ac5365: WARNING:at_mm/gup.c:#__get_user_pages

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Tue, 4 Jul 2023 at 00:03, kernel test robot <oliver.sang@intel.com> wrote:
>
> we noticed this commit 'add a (temporary) warning' for the case that
> 'anybody actually does anything quite this strange'.
> and in our this test, the warning hits. just FYI.

Yeah, so it looks like this is trinity doing system calls with random
arguments, and that will obviously hit the whole

  "GUP will no longer expand the stack, warn if somebody seems to want
to do GUP under the stack"

test.

So then it will warn if somebody passes in bogus addresses that *used*
to maybe work.

But with a random argument tester like trinity, passing in random
bogus addresses is obviously expected, so the warning will trigger
even if it's not something that we would not want to keep working.

I do not have a good idea for how to not warn for things like syzbot
and trinity that do random system calls, and only warn for any
potential real applications that do crazy things and expect them to
work.

And I *do* want the backtrace from the warning (in this case, it shows
that it's the "process_vm_readv/writev()" path, which actually might
be worth adding stack expansion to, the same way __access_remote_vm()
does).

I guess I can do the limiting manually, and just avoid WARN_ON_ONCE().

If I do just "dump_stack()", will the kernel test robot react to that
too? IOW, would a patch like the attached make the kernel test robot
not react?

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1230 bytes --]

 mm/gup.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index ef29641671c7..c9d799d28de7 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1091,6 +1091,21 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
 	return 0;
 }
 
+static void gup_stack_expansion_warning(const struct vm_area_struct *vma,
+	unsigned long addr)
+{
+	static volatile unsigned long next_warn;
+	unsigned long now = jiffies, next = next_warn;
+
+	/* Let's not warn more than once an hour.. */
+	if (next && time_before(now, next))
+		return;
+	next_warn = now + 60*60*HZ;
+	pr_warn("GUP no longer grows the stack %lx-%lx (%lx)\n",
+		vma->vm_start, vma->vm_end, addr);
+	dump_stack();
+}
+
 /**
  * __get_user_pages() - pin user pages in memory
  * @mm:		mm_struct of target mm
@@ -1170,7 +1185,8 @@ static long __get_user_pages(struct mm_struct *mm,
 		if (!vma || start >= vma->vm_end) {
 			vma = find_vma(mm, start);
 			if (vma && (start < vma->vm_start)) {
-				WARN_ON_ONCE(vma->vm_flags & VM_GROWSDOWN);
+				if (unlikely(vma->vm_flags & VM_GROWSDOWN))
+					gup_stack_expansion_warning(vma, start);
 				vma = NULL;
 			}
 			if (!vma && in_gate_area(mm, start)) {