Message-ID: <ZKUevnyH8EDM971n@xsang-OptiPlex-9020>
Date:   Wed, 5 Jul 2023 15:41:50 +0800
From:   Oliver Sang <oliver.sang@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     <oe-lkp@...ts.linux.dev>, <lkp@...el.com>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
        <oliver.sang@...el.com>
Subject: Re: [linus:master] [gup] a425ac5365:
 WARNING:at_mm/gup.c:#__get_user_pages

hi Linus,

On Tue, Jul 04, 2023 at 07:12:23AM -0700, Linus Torvalds wrote:
> On Tue, 4 Jul 2023 at 00:03, kernel test robot <oliver.sang@...el.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?

by applying below patch upon
"a425ac5365f6c gup: add warning if some caller would seem to want stack expansion"
then runing same trinity tests, we noticed there is no explict WARNING now,
instead, we saw below in dmesg (attached also):

[  323.996325][ T3994] GUP no longer grows the stack f7197000-f723e000 (f7196000)
[  323.997613][ T3994] CPU: 1 PID: 3994 Comm: trinity-c1 Not tainted 6.4.0-rc7-00014-ga7fb8f6e6830 #1
[  323.998883][ T3994] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[  324.000288][ T3994] Call Trace:
[  324.000829][ T3994]  <TASK>
[  324.001326][ T3994]  dump_stack_lvl+0xc5/0x140
[  324.002020][ T3994]  dump_stack+0xc/0x10
[  324.002653][ T3994]  __get_user_pages+0x78f/0x8d0
[  324.003399][ T3994]  __gup_longterm_locked+0xa2d/0xef0
[  324.004170][ T3994]  ? process_vm_rw+0x3c8/0x690
[  324.004873][ T3994]  ? process_vm_rw+0x3c8/0x690
[  324.005594][ T3994]  ? is_valid_gup_args+0x2a2/0x2b0
[  324.006349][ T3994]  pin_user_pages_remote+0x70/0xa0
[  324.007107][ T3994]  process_vm_rw+0x3f0/0x690
[  324.007842][ T3994]  ? __ct_user_exit+0x57/0x70
[  324.008543][ T3994]  __ia32_sys_process_vm_readv+0x75/0xa0
[  324.009362][ T3994]  __do_fast_syscall_32+0xed/0x130
[  324.010116][ T3994]  ? __do_fast_syscall_32+0x108/0x130
[  324.010902][ T3994]  ? __do_fast_syscall_32+0x108/0x130
[  324.011690][ T3994]  ? __do_fast_syscall_32+0x108/0x130
[  324.012469][ T3994]  ? irqentry_exit_to_user_mode+0x23/0x40
[  324.013295][ T3994]  ? irqentry_exit+0x6d/0xc0
[  324.014002][ T3994]  do_fast_syscall_32+0x2f/0x70
[  324.014723][ T3994]  do_SYSENTER_32+0x17/0x20
[  324.015416][ T3994]  entry_SYSENTER_compat_after_hwframe+0x53/0x62
[  324.016311][ T3994] RIP: 0023:0xf7fb3539
[  324.016942][ T3994] Code: 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd
80 <5d> 5a 59 c3 90 90 90 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00
[  324.019421][ T3994] RSP: 002b:00000000ffa1c20c EFLAGS: 00000292 ORIG_RAX: 000000000000015b
[  324.020595][ T3994] RAX: ffffffffffffffda RBX: 0000000000000f9a RCX: 0000000057150a40
[  324.021721][ T3994] RDX: 0000000000000001 RSI: 00000000571509d0 RDI: 0000000000000001
[  324.022834][ T3994] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[  324.023981][ T3994] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  324.025108][ T3994] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  324.026255][ T3994]  </TASK>


> 
>               Linus

>  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)) {


Download attachment "dmesg.xz" of type "application/x-xz" (20356 bytes)

Powered by blists - more mailing lists