On Wed, Oct 21, 2020 at 9:58 AM Naresh Kamboju
<[email protected]> wrote:
>
> LTP mm mtest05 (mmstress), mtest06_3 and mallocstress01 (mallocstress) tested on
> x86 KASAN enabled build. But tests are getting PASS on Non KASAN builds.
> This regression started happening from next-20201015 nowards
Is it repeatable enough to be bisectable?
Linus
On Wed, 21 Oct 2020 at 22:35, Linus Torvalds
<[email protected]> wrote:
>
> On Wed, Oct 21, 2020 at 9:58 AM Naresh Kamboju
> <[email protected]> wrote:
> >
> > LTP mm mtest05 (mmstress), mtest06_3 and mallocstress01 (mallocstress) tested on
> > x86 KASAN enabled build. But tests are getting PASS on Non KASAN builds.
> > This regression started happening from next-20201015 nowards
>
> Is it repeatable enough to be bisectable?
Yes. This is easily reproducible.
I will bisect and report here.
>
> Linus
- Naresh
On Wed, 21 Oct 2020 at 22:52, Naresh Kamboju <[email protected]> wrote:
>
> On Wed, 21 Oct 2020 at 22:35, Linus Torvalds
> <[email protected]> wrote:
> >
> > On Wed, Oct 21, 2020 at 9:58 AM Naresh Kamboju
> > <[email protected]> wrote:
> > >
> > > LTP mm mtest05 (mmstress), mtest06_3 and mallocstress01 (mallocstress) tested on
> > > x86 KASAN enabled build. But tests are getting PASS on Non KASAN builds.
> > > This regression started happening from next-20201015 nowards
> >
> > Is it repeatable enough to be bisectable?
>
> Yes. This is easily reproducible.
> I will bisect and report here.
The bad commit points to,
commit d55564cfc222326e944893eff0c4118353e349ec
x86: Make __put_user() generate an out-of-line call
I have reverted this single patch and confirmed the reported
problem is not seen anymore.
- Naresh
On Thu, Oct 22, 2020 at 1:55 PM Naresh Kamboju
<[email protected]> wrote:
>
> The bad commit points to,
>
> commit d55564cfc222326e944893eff0c4118353e349ec
> x86: Make __put_user() generate an out-of-line call
>
> I have reverted this single patch and confirmed the reported
> problem is not seen anymore.
Thanks. Very funky, but thanks. I've been running that commit on my
machine for over half a year, and it still looks "trivially correct"
to me, but let me go look at it one more time. Can't argue with a
reliable bisect and revert..
Linus
On Thu, Oct 22, 2020 at 4:43 PM Linus Torvalds
<[email protected]> wrote:
>
> Thanks. Very funky, but thanks. I've been running that commit on my
> machine for over half a year, and it still looks "trivially correct"
> to me, but let me go look at it one more time. Can't argue with a
> reliable bisect and revert..
Hmm. The fact that it only happens with KASAN makes me suspect it's
some bad interaction with the inline asm syntax change (and explains
why I've run with this for half a year without issues).
In particular, I wonder if it's that KASAN causes some reload pattern,
and the whole
register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);
..
asm volatile(.. "r" (__val_pu) ..)
thing causes problems. That's an ugly pattern, but it's written that
way to get gcc to handle the 64-bit case properly (with the value in
%rax:%rdx).
It turns out that the decode of the user-mode SIGSEGV code is a
variation of system calls, ie
0: b8 18 00 00 00 mov $0x18,%eax
5: 0f 05 syscall
7: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
d: 73 01 jae 0x10
f:* c3 retq <-- trapping instruction
or
0: 41 52 push %r10
2: 52 push %rdx
3: 4d 31 d2 xor %r10,%r10
6: ba 02 00 00 00 mov $0x2,%edx
b: be 80 00 00 00 mov $0x80,%esi
10: 39 d0 cmp %edx,%eax
12: 75 07 jne 0x1b
14: b8 ca 00 00 00 mov $0xca,%eax
19: 0f 05 syscall
1b: 89 d0 mov %edx,%eax
1d: 87 07 xchg %eax,(%rdi)
1f: 85 c0 test %eax,%eax
21: 75 f1 jne 0x14
23:* 5a pop %rdx <-- trapping instruction
24: 41 5a pop %r10
26: c3 retq
so in both cases it looks like 'syscall' returned with a bad stack pointer.
Which is certainly a sign of some code generation issue.
Very annoying, because it probably means that it's compiler-specific
too. And that "syscall 018" looks very odd. I think that's
sched_yield() on x86-64, which doesn't have any __put_user() cases at
all..
Would you mind sending me the problematic vmlinux file in private (or,
likely better - a pointer to some place I can download it, it's going
to be huge).
Linus
Hello!
On Thu, 22 Oct 2020 at 19:11, Linus Torvalds
<[email protected]> wrote:
> On Thu, Oct 22, 2020 at 4:43 PM Linus Torvalds
> Would you mind sending me the problematic vmlinux file in private (or,
> likely better - a pointer to some place I can download it, it's going
> to be huge).
The kernel Naresh originally referred to is here:
https://builds.tuxbuild.com/SCI7Xyjb7V2NbfQ2lbKBZw/
Greetings!
Daniel Díaz
[email protected]
On Thu, Oct 22, 2020 at 5:11 PM Linus Torvalds
<[email protected]> wrote:
>
> In particular, I wonder if it's that KASAN causes some reload pattern,
> and the whole
>
> register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX);
> ..
> asm volatile(.. "r" (__val_pu) ..)
>
> thing causes problems.
That pattern isn't new (see the same pattern and the comment above get_user).
But our previous use of that pattern had it as an output of the asm,
and the new use is as an input. That obviously shouldn't matter, but
if it's some odd compiler code generation interaction, all bets are
off..
Linus
On Thu, Oct 22, 2020 at 6:36 PM Daniel Díaz <[email protected]> wrote:
>
> The kernel Naresh originally referred to is here:
> https://builds.tuxbuild.com/SCI7Xyjb7V2NbfQ2lbKBZw/
Thanks.
And when I started looking at it, I realized that my original idea
("just look for __put_user_nocheck_X calls, there aren't so many of
those") was garbage, and that I was just being stupid.
Yes, the commit that broke was about __put_user(), but in order to not
duplicate all the code, it re-used the regular put_user()
infrastructure, and so all the normal put_user() calls are potential
problem spots too if this is about the compiler interaction with KASAN
and the asm changes.
So it's not just a couple of special cases to look at, it's all the
normal cases too.
Ok, back to the drawing board, but I think reverting it is probably
the right thing to do if I can't think of something smart.
That said, since you see this on x86-64, where the whole ugly trick with that
register asm("%"_ASM_AX)
is unnecessary (because the 8-byte case is still just a single
register, no %eax:%edx games needed), it would be interesting to hear
if the attached patch fixes it. That would confirm that the problem
really is due to some register allocation issue interaction (or,
alternatively, it would tell me that there's something else going on).
Linus
On Fri, 23 Oct 2020 at 08:35, Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Oct 22, 2020 at 6:36 PM Daniel Díaz <[email protected]> wrote:
> >
> > The kernel Naresh originally referred to is here:
> > https://builds.tuxbuild.com/SCI7Xyjb7V2NbfQ2lbKBZw/
>
> is unnecessary (because the 8-byte case is still just a single
> register, no %eax:%edx games needed), it would be interesting to hear
> if the attached patch fixes it. That would confirm that the problem
> really is due to some register allocation issue interaction (or,
> alternatively, it would tell me that there's something else going on).
[Old patch from yesterday]
After applying your patch on top on linux next tag 20201015
there are two observations,
1) i386 build failed. please find build error build
2) x86_64 kasan test PASS and the reported error not found.
i386 build failure,
----------------------
make -sk KBUILD_BUILD_USER=TuxBuild -C/linux -j16 ARCH=i386 HOSTCC=gcc
CC="sccache gcc" O=build
#
In file included from ../include/linux/uaccess.h:11,
from ../arch/x86/include/asm/fpu/xstate.h:5,
from ../arch/x86/include/asm/pgtable.h:26,
from ../include/linux/pgtable.h:6,
from ../include/linux/mm.h:33,
from ../include/linux/memblock.h:13,
from ../fs/proc/page.c:2:
../fs/proc/page.c: In function ‘kpagecgroup_read’:
../arch/x86/include/asm/uaccess.h:217:2: error: inconsistent operand
constraints in an ‘asm’
217 | asm volatile("call __" #fn "_%P[size]" \
| ^~~
../arch/x86/include/asm/uaccess.h:244:44: note: in expansion of macro
‘do_put_user_call’
244 | #define put_user(x, ptr) ({ might_fault();
do_put_user_call(put_user,x,ptr); })
| ^~~~~~~~~~~~~~~~
../fs/proc/page.c:307:7: note: in expansion of macro ‘put_user’
307 | if (put_user(ino, out)) {
| ^~~~~~~~
make[3]: *** [../scripts/Makefile.build:283: fs/proc/page.o] Error 1
make[3]: Target '__build' not remade because of errors.
make[2]: *** [../scripts/Makefile.build:500: fs/proc] Error 2
In file included from ../include/linux/uaccess.h:11,
from ../include/linux/sched/task.h:11,
from ../include/linux/sched/signal.h:9,
from ../include/linux/rcuwait.h:6,
from ../include/linux/percpu-rwsem.h:7,
from ../include/linux/fs.h:33,
from ../include/linux/cgroup.h:17,
from ../include/linux/memcontrol.h:13,
from ../include/linux/swap.h:9,
from ../include/linux/suspend.h:5,
from ../kernel/power/user.c:10:
../kernel/power/user.c: In function ‘snapshot_ioctl’:
../arch/x86/include/asm/uaccess.h:217:2: error: inconsistent operand
constraints in an ‘asm’
217 | asm volatile("call __" #fn "_%P[size]" \
| ^~~
../arch/x86/include/asm/uaccess.h:244:44: note: in expansion of macro
‘do_put_user_call’
244 | #define put_user(x, ptr) ({ might_fault();
do_put_user_call(put_user,x,ptr); })
| ^~~~~~~~~~~~~~~~
../kernel/power/user.c:340:11: note: in expansion of macro ‘put_user’
340 | error = put_user(size, (loff_t __user *)arg);
| ^~~~~~~~
../arch/x86/include/asm/uaccess.h:217:2: error: inconsistent operand
constraints in an ‘asm’
217 | asm volatile("call __" #fn "_%P[size]" \
| ^~~
../arch/x86/include/asm/uaccess.h:244:44: note: in expansion of macro
‘do_put_user_call’
244 | #define put_user(x, ptr) ({ might_fault();
do_put_user_call(put_user,x,ptr); })
| ^~~~~~~~~~~~~~~~
../kernel/power/user.c:346:11: note: in expansion of macro ‘put_user’
346 | error = put_user(size, (loff_t __user *)arg);
| ^~~~~~~~
../arch/x86/include/asm/uaccess.h:217:2: error: inconsistent operand
constraints in an ‘asm’
217 | asm volatile("call __" #fn "_%P[size]" \
| ^~~
../arch/x86/include/asm/uaccess.h:244:44: note: in expansion of macro
‘do_put_user_call’
244 | #define put_user(x, ptr) ({ might_fault();
do_put_user_call(put_user,x,ptr); })
| ^~~~~~~~~~~~~~~~
../kernel/power/user.c:357:12: note: in expansion of macro ‘put_user’
357 | error = put_user(offset, (loff_t __user *)arg);
| ^~~~~~~~
x86_64 Kasan tested and the reported issue not found.
https://lkft.validation.linaro.org/scheduler/job/1868029#L2374
- Naresh
On Fri, Oct 23, 2020 at 10:00 AM Naresh Kamboju
<[email protected]> wrote:
>
> [Old patch from yesterday]
>
> After applying your patch on top on linux next tag 20201015
> there are two observations,
> 1) i386 build failed. please find build error build
Yes, this was expected. That patch explicitly only works on x86-64,
because 32-bit needs the double register handling for 64-bit values
(mainly loff_t).
> 2) x86_64 kasan test PASS and the reported error not found.
Ok, good. That confirms that the problem you reported is indeed the
register allocation.
The patch I sent an hour ago (the one based on Rasmus' one from
yesterday) should fix things too, and - unlike yesterday's - work on
32-bit.
But I'll wait for confirmation (and hopefully a sign-off from Rasmus
so that I can give him authorship) before actually committing it.
Linus
On Fri, Oct 23, 2020 at 10:51 AM Linus Torvalds
<[email protected]> wrote:
>
> On Fri, Oct 23, 2020 at 10:00 AM Naresh Kamboju
> <[email protected]> wrote:
> >
> > [Old patch from yesterday]
> >
> > After applying your patch on top on linux next tag 20201015
> > there are two observations,
> > 1) i386 build failed. please find build error build
>
> Yes, this was expected. That patch explicitly only works on x86-64,
> because 32-bit needs the double register handling for 64-bit values
> (mainly loff_t).
>
> > 2) x86_64 kasan test PASS and the reported error not found.
>
> Ok, good. That confirms that the problem you reported is indeed the
> register allocation.
>
> The patch I sent an hour ago (the one based on Rasmus' one from
> yesterday) should fix things too, and - unlike yesterday's - work on
> 32-bit.
>
> But I'll wait for confirmation (and hopefully a sign-off from Rasmus
> so that I can give him authorship) before actually committing it.
>
> Linus
My test vm failed to boot since
commit d55564cfc222326e944893eff0c4118353e349ec
x86: Make __put_user() generate an out-of-line call
The patch also fixed it.
Thanks!
Song