Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-11-03 Thread Michael Ellerman 
Andreas Schwab  writes:
> #
> # Automatically generated file; DO NOT EDIT.
> # Linux/powerpc 5.10.0-rc1 Kernel Configuration
> #
> CONFIG_CC_VERSION_TEXT="gcc-4.9 (SUSE Linux) 4.9.3"

So it seems to be a combination of GCC 4.9 and ...

> # CONFIG_PPC_RADIX_MMU is not set

That ^, which specifically causes PPC_KUAP=n.

When PPC_KUAP=y allow_user_access() inlines an isync and mtspr, both of
which contain a memory clobber, and that seems to hide the bug.

I think for now we just have to stop using asm goto for put_user() on
GCC 4.9.

I'll send a patch for that.

cheers

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-10-29 Thread Andreas Schwab 
#
# Automatically generated file; DO NOT EDIT.
# Linux/powerpc 5.10.0-rc1 Kernel Configuration
#
CONFIG_CC_VERSION_TEXT="gcc-4.9 (SUSE Linux) 4.9.3"
CONFIG_CC_IS_GCC=y
CONFIG_GCC_VERSION=40903
CONFIG_LD_VERSION=23501
CONFIG_CLANG_VERSION=0
CONFIG_CC_CAN_LINK=y
CONFIG_CC_CAN_LINK_STATIC=y
CONFIG_CC_HAS_ASM_GOTO=y
CONFIG_IRQ_WORK=y
CONFIG_BUILDTIME_TABLE_SORT=y
CONFIG_THREAD_INFO_IN_TASK=y

#
# General setup
#
CONFIG_INIT_ENV_ARG_LIMIT=32
# CONFIG_COMPILE_TEST is not set
CONFIG_LOCALVERSION=""
# CONFIG_LOCALVERSION_AUTO is not set
CONFIG_BUILD_SALT=""
CONFIG_HAVE_KERNEL_GZIP=y
CONFIG_HAVE_KERNEL_XZ=y
CONFIG_KERNEL_GZIP=y
# CONFIG_KERNEL_XZ is not set
CONFIG_DEFAULT_INIT=""
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
CONFIG_WATCH_QUEUE=y
CONFIG_CROSS_MEMORY_ATTACH=y
CONFIG_USELIB=y
CONFIG_AUDIT=y
CONFIG_HAVE_ARCH_AUDITSYSCALL=y
CONFIG_AUDITSYSCALL=y

#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_GENERIC_IRQ_SHOW_LEVEL=y
CONFIG_GENERIC_IRQ_MIGRATION=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_FORCED_THREADING=y
CONFIG_SPARSE_IRQ=y
# CONFIG_GENERIC_IRQ_DEBUGFS is not set
# end of IRQ subsystem

CONFIG_GENERIC_TIME_VSYSCALL=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_ARCH_HAS_TICK_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CMOS_UPDATE=y

#
# Timers subsystem
#
CONFIG_TICK_ONESHOT=y
CONFIG_NO_HZ_COMMON=y
# CONFIG_HZ_PERIODIC is not set
CONFIG_NO_HZ_IDLE=y
# CONFIG_NO_HZ_FULL is not set
CONFIG_NO_HZ=y
CONFIG_HIGH_RES_TIMERS=y
# end of Timers subsystem

# CONFIG_PREEMPT_NONE is not set
CONFIG_PREEMPT_VOLUNTARY=y
# CONFIG_PREEMPT is not set

#
# CPU/Task time and stats accounting
#
CONFIG_VIRT_CPU_ACCOUNTING=y
# CONFIG_TICK_CPU_ACCOUNTING is not set
CONFIG_VIRT_CPU_ACCOUNTING_NATIVE=y
# CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set
CONFIG_BSD_PROCESS_ACCT=y
CONFIG_BSD_PROCESS_ACCT_V3=y
CONFIG_TASKSTATS=y
CONFIG_TASK_DELAY_ACCT=y
CONFIG_TASK_XACCT=y
CONFIG_TASK_IO_ACCOUNTING=y
CONFIG_PSI=y
# CONFIG_PSI_DEFAULT_DISABLED is not set
# end of CPU/Task time and stats accounting

# CONFIG_CPU_ISOLATION is not set

#
# RCU Subsystem
#
CONFIG_TREE_RCU=y
# CONFIG_RCU_EXPERT is not set
CONFIG_SRCU=y
CONFIG_TREE_SRCU=y
CONFIG_TASKS_RCU_GENERIC=y
CONFIG_TASKS_TRACE_RCU=y
CONFIG_RCU_STALL_COMMON=y
CONFIG_RCU_NEED_SEGCBLIST=y
# end of RCU Subsystem

CONFIG_BUILD_BIN2C=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
# CONFIG_IKHEADERS is not set
CONFIG_LOG_BUF_SHIFT=18
CONFIG_LOG_CPU_MAX_BUF_SHIFT=15
CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT=13

#
# Scheduler features
#
CONFIG_UCLAMP_TASK=y
CONFIG_UCLAMP_BUCKETS_COUNT=5
# end of Scheduler features

CONFIG_ARCH_SUPPORTS_NUMA_BALANCING=y
CONFIG_CC_HAS_INT128=y
CONFIG_CGROUPS=y
CONFIG_PAGE_COUNTER=y
CONFIG_MEMCG=y
CONFIG_MEMCG_SWAP=y
CONFIG_MEMCG_KMEM=y
CONFIG_BLK_CGROUP=y
CONFIG_CGROUP_WRITEBACK=y
CONFIG_CGROUP_SCHED=y
CONFIG_FAIR_GROUP_SCHED=y
CONFIG_CFS_BANDWIDTH=y
CONFIG_RT_GROUP_SCHED=y
# CONFIG_UCLAMP_TASK_GROUP is not set
CONFIG_CGROUP_PIDS=y
CONFIG_CGROUP_RDMA=y
CONFIG_CGROUP_FREEZER=y
CONFIG_CGROUP_HUGETLB=y
CONFIG_CPUSETS=y
CONFIG_PROC_PID_CPUSET=y
CONFIG_CGROUP_DEVICE=y
CONFIG_CGROUP_CPUACCT=y
CONFIG_CGROUP_PERF=y
CONFIG_CGROUP_BPF=y
# CONFIG_CGROUP_DEBUG is not set
CONFIG_SOCK_CGROUP_DATA=y
CONFIG_NAMESPACES=y
CONFIG_UTS_NS=y
CONFIG_IPC_NS=y
CONFIG_USER_NS=y
CONFIG_PID_NS=y
CONFIG_NET_NS=y
CONFIG_CHECKPOINT_RESTORE=y
# CONFIG_SCHED_AUTOGROUP is not set
# CONFIG_SYSFS_DEPRECATED is not set
CONFIG_RELAY=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_INITRAMFS_SOURCE=""
CONFIG_RD_GZIP=y
CONFIG_RD_BZIP2=y
CONFIG_RD_LZMA=y
CONFIG_RD_XZ=y
CONFIG_RD_LZO=y
CONFIG_RD_LZ4=y
CONFIG_RD_ZSTD=y
CONFIG_BOOT_CONFIG=y
# CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE is not set
CONFIG_CC_OPTIMIZE_FOR_SIZE=y
CONFIG_HAVE_LD_DEAD_CODE_DATA_ELIMINATION=y
# CONFIG_LD_DEAD_CODE_DATA_ELIMINATION is not set
CONFIG_SYSCTL=y
CONFIG_SYSCTL_EXCEPTION_TRACE=y
CONFIG_BPF=y
CONFIG_EXPERT=y
CONFIG_MULTIUSER=y
CONFIG_SGETMASK_SYSCALL=y
CONFIG_SYSFS_SYSCALL=y
CONFIG_FHANDLE=y
CONFIG_POSIX_TIMERS=y
CONFIG_PRINTK=y
CONFIG_PRINTK_NMI=y
CONFIG_BUG=y
CONFIG_ELF_CORE=y
CONFIG_BASE_FULL=y
CONFIG_FUTEX=y
CONFIG_FUTEX_PI=y
CONFIG_EPOLL=y
CONFIG_SIGNALFD=y
CONFIG_TIMERFD=y
CONFIG_EVENTFD=y
CONFIG_SHMEM=y
CONFIG_AIO=y
CONFIG_IO_URING=y
CONFIG_ADVISE_SYSCALLS=y
CONFIG_MEMBARRIER=y
CONFIG_KALLSYMS=y
CONFIG_KALLSYMS_ALL=y
CONFIG_KALLSYMS_BASE_RELATIVE=y
CONFIG_BPF_SYSCALL=y
CONFIG_USERMODE_DRIVER=y
# CONFIG_BPF_PRELOAD is not set
CONFIG_USERFAULTFD=y
CONFIG_ARCH_HAS_MEMBARRIER_CALLBACKS=y
CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE=y
CONFIG_RSEQ=y
# CONFIG_DEBUG_RSEQ is not set
# CONFIG_EMBEDDED is not set
CONFIG_HAVE_PERF_EVENTS=y
# CONFIG_PC104 is not set

#
# Kernel Performance Events And Counters
#
CONFIG_PERF_EVENTS=y
# end of Kernel Performance Events And Counters

CONFIG_VM_EVENT_COUNTERS=y
CONFIG_SLUB_DEBUG=y
CONFIG_SLUB_MEMCG_SYSFS_ON=y
# CONFIG_COMPAT_BRK is not set
# CONFIG_SLAB is not set
CONFIG_SLUB=y
# CONFIG_SLOB is not set

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-10-28 Thread Michael Ellerman 
Andreas Schwab  writes:
> On Okt 28 2020, Andreas Schwab wrote:
>
>> On Sep 04 2020, Christophe Leroy wrote:
>>
>>> __put_user_asm_goto() provides more flexibility to GCC and avoids using
>>> a local variable to tell if the write succeeded or not.
>>> GCC can then avoid implementing a cmp in the fast path.
>>
>> That breaks CLONE_CHILD_SETTID.  I'm getting an assertion failure in
>> __libc_fork (THREAD_GETMEM (self, tid) != ppid).
>
> This is what schedule_tail now looks like.  As you can see, put_user has
> become a nop:
>
> 455c <.schedule_tail>:
> 455c:   7c 08 02 a6 mflrr0
> 4560:   f8 01 00 10 std r0,16(r1)
> 4564:   f8 21 ff 91 stdur1,-112(r1)
> 4568:   4b ff cd 4d bl  12b4 <.finish_task_switch>
> 456c:   4b ff c0 99 bl  604 <.balance_callback>
> 4570:   e8 6d 01 88 ld  r3,392(r13)
> 4574:   e9 23 06 b0 ld  r9,1712(r3)
> 4578:   2f a9 00 00 cmpdi   cr7,r9,0
> 457c:   41 9e 00 14 beq cr7,4590 <.schedule_tail+0x34>
> 4580:   38 80 00 00 li  r4,0
> 4584:   38 a0 00 00 li  r5,0
> 4588:   48 00 00 01 bl  4588 <.schedule_tail+0x2c>
> 4588: R_PPC64_REL24 .__task_pid_nr_ns
> 458c:   60 00 00 00 nop
> 4590:   48 00 00 01 bl  4590 <.schedule_tail+0x34>
> 4590: R_PPC64_REL24 .calculate_sigpending
> 4594:   60 00 00 00 nop
> 4598:   38 21 00 70 addir1,r1,112
> 459c:   e8 01 00 10 ld  r0,16(r1)
> 45a0:   7c 08 03 a6 mtlrr0
> 45a4:   4e 80 00 20 blr

Not for me, see below.

What config and compiler are you using?

cheers



c0181aa0 :
c0181aa0:   82 01 4c 3c addis   r2,r12,386
c0181aa4:   60 1b 42 38 addir2,r2,7008
c0181aa8:   a6 02 08 7c mflrr0
c0181aac:   cd b5 ee 4b bl  c006d078 <_mcount>
c0181ab0:   a6 02 08 7c mflrr0
c0181ab4:   f8 ff e1 fb std r31,-8(r1)
c0181ab8:   10 00 01 f8 std r0,16(r1)
c0181abc:   d1 ff 21 f8 stdur1,-48(r1)
c0181ac0:   c9 7d ff 4b bl  c0179888 

c0181ac4:   40 0a 23 e9 ld  r9,2624(r3)
c0181ac8:   00 00 a9 2f cmpdi   cr7,r9,0
c0181acc:   b4 00 9e 40 bne cr7,c0181b80 

c0181ad0:   68 09 6d e8 ld  r3,2408(r13)
c0181ad4:   48 07 e3 eb ld  r31,1864(r3)
c0181ad8:   00 00 bf 2f cmpdi   cr7,r31,0
c0181adc:   88 00 9e 41 beq cr7,c0181b64 

c0181ae0:   00 00 a0 38 li  r5,0
c0181ae4:   00 00 80 38 li  r4,0
c0181ae8:   21 4b fe 4b bl  c0166608 
<__task_pid_nr_ns+0x8>
c0181aec:   00 00 00 60 nop
c0181af0:   ff ff 20 39 li  r9,-1
c0181af4:   00 03 29 79 clrldi  r9,r9,12
c0181af8:   40 48 bf 7f cmpld   cr7,r31,r9
c0181afc:   68 00 9d 41 bgt cr7,c0181b64 

c0181b00:   01 00 29 39 addir9,r9,1
c0181b04:   50 48 3f 7d subfr9,r31,r9
c0181b08:   03 00 a9 2b cmpldi  cr7,r9,3
c0181b0c:   58 00 9d 40 ble cr7,c0181b64 

c0181b10:   02 00 42 3d addis   r10,r2,2
c0181b14:   18 25 4a 39 addir10,r10,9496
c0181b18:   00 00 2a e9 ld  r9,0(r10)
c0181b1c:   22 00 29 e9 lwa r9,32(r9)
c0181b20:   00 00 89 2f cmpwi   cr7,r9,0
c0181b24:   24 00 9c 40 bge cr7,c0181b48 

c0181b28:   2c 01 00 4c isync
c0181b2c:   00 40 20 3d lis r9,16384
c0181b30:   c6 07 29 79 rldicr  r9,r9,32,31

c0181b34:   a6 03 3d 7d mtspr   29,r9   # put_user() 
begins here
c0181b38:   2c 01 00 4c isync
c0181b3c:   00 00 2a e9 ld  r9,0(r10)
c0181b40:   22 00 29 e9 lwa r9,32(r9)
c0181b44:   00 00 89 2f cmpwi   cr7,r9,0
c0181b48:   00 00 7f 90 stw r3,0(r31)
c0181b4c:   18 00 9c 40 bge cr7,c0181b64 

c0181b50:   2c 01 00 4c isync
c0181b54:   ff ff 20 39 li  r9,-1
c0181b58:   44 00 29 79 rldicr  r9,r9,0,1
c0181b5c:   a6 03 3d 7d mtspr   29,r9
c0181b60:   2c 01 00 4c isync

c0181b64:   b5 c9 fc 4b bl  c014e518 

c0181b68:   00 00 00 60 nop
c0181b6c:   30 00 21 38 addir1,r1,48
c0181b70:   10 00 01 e8 ld  r0,16(r1)
c0181b74:   f8 ff e1 eb

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-10-28 Thread Andreas Schwab 
On Okt 28 2020, Michael Ellerman wrote:

> What config and compiler are you using?

gcc 4.9.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-10-27 Thread Andreas Schwab 
On Okt 28 2020, Andreas Schwab wrote:

> On Sep 04 2020, Christophe Leroy wrote:
>
>> __put_user_asm_goto() provides more flexibility to GCC and avoids using
>> a local variable to tell if the write succeeded or not.
>> GCC can then avoid implementing a cmp in the fast path.
>
> That breaks CLONE_CHILD_SETTID.  I'm getting an assertion failure in
> __libc_fork (THREAD_GETMEM (self, tid) != ppid).

This is what schedule_tail now looks like.  As you can see, put_user has
become a nop:

455c <.schedule_tail>:
455c:   7c 08 02 a6 mflrr0
4560:   f8 01 00 10 std r0,16(r1)
4564:   f8 21 ff 91 stdur1,-112(r1)
4568:   4b ff cd 4d bl  12b4 <.finish_task_switch>
456c:   4b ff c0 99 bl  604 <.balance_callback>
4570:   e8 6d 01 88 ld  r3,392(r13)
4574:   e9 23 06 b0 ld  r9,1712(r3)
4578:   2f a9 00 00 cmpdi   cr7,r9,0
457c:   41 9e 00 14 beq cr7,4590 <.schedule_tail+0x34>
4580:   38 80 00 00 li  r4,0
4584:   38 a0 00 00 li  r5,0
4588:   48 00 00 01 bl  4588 <.schedule_tail+0x2c>
4588: R_PPC64_REL24 .__task_pid_nr_ns
458c:   60 00 00 00 nop
4590:   48 00 00 01 bl  4590 <.schedule_tail+0x34>
4590: R_PPC64_REL24 .calculate_sigpending
4594:   60 00 00 00 nop
4598:   38 21 00 70 addir1,r1,112
459c:   e8 01 00 10 ld  r0,16(r1)
45a0:   7c 08 03 a6 mtlrr0
45a4:   4e 80 00 20 blr

This is schedule_tail in 5.9:

455c <.schedule_tail>:
455c:   7c 08 02 a6 mflrr0
4560:   fb c1 ff f0 std r30,-16(r1)
4564:   fb e1 ff f8 std r31,-8(r1)
4568:   f8 01 00 10 std r0,16(r1)
456c:   f8 21 ff 81 stdur1,-128(r1)
4570:   4b ff cd 45 bl  12b4 <.finish_task_switch>
4574:   4b ff c0 91 bl  604 <.balance_callback>
4578:   eb cd 01 88 ld  r30,392(r13)
457c:   eb fe 06 b0 ld  r31,1712(r30)
4580:   2f bf 00 00 cmpdi   cr7,r31,0
4584:   41 9e 00 2c beq cr7,45b0 <.schedule_tail+0x54>
4588:   7f c3 f3 78 mr  r3,r30
458c:   38 80 00 00 li  r4,0
4590:   38 a0 00 00 li  r5,0
4594:   48 00 00 01 bl  4594 <.schedule_tail+0x38>
4594: R_PPC64_REL24 .__task_pid_nr_ns
4598:   60 00 00 00 nop
459c:   e9 3e 0a b8 ld  r9,2744(r30)
45a0:   7f bf 48 40 cmpld   cr7,r31,r9
45a4:   41 9d 00 0c bgt cr7,45b0 <.schedule_tail+0x54>
45a8:   2b a9 00 03 cmpldi  cr7,r9,3
45ac:   41 9d 00 14 bgt cr7,45c0 <.schedule_tail+0x64>
45b0:   48 00 00 01 bl  45b0 <.schedule_tail+0x54>
45b0: R_PPC64_REL24 .calculate_sigpending
45b4:   60 00 00 00 nop
45b8:   38 21 00 80 addir1,r1,128
45bc:   48 00 00 00 b   45bc <.schedule_tail+0x60>
45bc: R_PPC64_REL24 _restgpr0_30
45c0:   39 20 00 00 li  r9,0
45c4:   90 7f 00 00 stw r3,0(r31)
45c8:   4b ff ff e8 b   45b0 <.schedule_tail+0x54>


Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-10-27 Thread Andreas Schwab 
On Sep 04 2020, Christophe Leroy wrote:

> __put_user_asm_goto() provides more flexibility to GCC and avoids using
> a local variable to tell if the write succeeded or not.
> GCC can then avoid implementing a cmp in the fast path.

That breaks CLONE_CHILD_SETTID.  I'm getting an assertion failure in
__libc_fork (THREAD_GETMEM (self, tid) != ppid).

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()

2020-09-17 Thread Michael Ellerman 
On Fri, 4 Sep 2020 11:01:30 + (UTC), Christophe Leroy wrote:
> __put_user_asm_goto() provides more flexibility to GCC and avoids using
> a local variable to tell if the write succeeded or not.
> GCC can then avoid implementing a cmp in the fast path.
> 
> See the difference for a small function like the PPC64 version of
> save_general_regs() in arch/powerpc/kernel/signal_32.c:
> 
> [...]

Applied to powerpc/next.

[1/3] powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()
  https://git.kernel.org/powerpc/c/ee0a49a6870ea75e25b4d4984c1bb6b3b7c65f2b
[2/3] powerpc/uaccess: Switch __patch_instruction() to __put_user_asm_goto()
  https://git.kernel.org/powerpc/c/e64ac41ab0c510b3f85199a585eb886cad92fb19
[3/3] powerpc/uaccess: Remove __put_user_asm() and __put_user_asm2()
  https://git.kernel.org/powerpc/c/7fdf966bed155b214f4f1f9b67825a40b2e9b998

cheers