From: Palmer Dabbelt <palmer@dabbelt.com> To: james.hogan@imgtec.com Cc: peterz@infradead.org Cc: mingo@redhat.com Cc: mcgrof@kernel.org Cc: viro@zeniv.linux.org.uk Cc: sfr@canb.auug.org.au Cc: nicolas.dichtel@6wind.com Cc: rmk+kernel@armlinux.org.uk Cc: msalter@redhat.com Cc: tklauser@distanz.ch Cc: will.deacon@arm.com Cc: paul.gortmaker@windriver.com Cc: linux@roeck-us.net Cc: linux-kernel@vger.kernel.org Cc: linux-arch@vger.kernel.org Cc: albert@sifive.com Subject: Re: [PATCH 8/9] RISC-V: User-facing API Date: Thu, 29 Jun 2017 14:42:38 -0700 (PDT) [thread overview] Message-ID: <mhng-21eaeea1-4cd1-43b1-9b2b-b664cea12d43@palmer-si-x1c4> (raw) In-Reply-To: <20170628224237.GO6973@jhogan-linux.le.imgtec.org> On Wed, 28 Jun 2017 15:42:37 PDT (-0700), james.hogan@imgtec.com wrote: > Hi Palmer, > > On Wed, Jun 28, 2017 at 11:55:37AM -0700, Palmer Dabbelt wrote: >> diff --git a/arch/riscv/include/asm/syscalls.h b/arch/riscv/include/asm/syscalls.h >> new file mode 100644 >> index 000000000000..d85267c4f7ea >> --- /dev/null >> +++ b/arch/riscv/include/asm/syscalls.h >> @@ -0,0 +1,25 @@ > ... >> +/* kernel/sys_riscv.c */ >> +asmlinkage long sys_sysriscv(unsigned long, unsigned long, >> + unsigned long, unsigned long); > > You suggested in the cover letter this wasn't muxed any longer, maybe > you should have a prototype for each of the cmpxchg syscalls instead? Sorry, I just missed that. I'll fix it for the v4 diff --git a/arch/riscv/include/asm/syscalls.h b/arch/riscv/include/asm/syscalls.h index d85267c4f7ea..6490274fbb76 100644 --- a/arch/riscv/include/asm/syscalls.h +++ b/arch/riscv/include/asm/syscalls.h @@ -19,7 +19,7 @@ #include <asm-generic/syscalls.h> /* kernel/sys_riscv.c */ -asmlinkage long sys_sysriscv(unsigned long, unsigned long, - unsigned long, unsigned long); +asmlinkage long sys_sysriscv_cmpxchg32(u32 __user * ptr, u32 new, u32 old); +asmlinkage long sys_sysriscv_cmpxchg64(u64 __user * ptr, u64 new, u64 old); #endif /* _ASM_RISCV_SYSCALLS_H */ >> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h >> new file mode 100644 >> index 000000000000..01aee1654eae >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/ptrace.h > ... >> +struct __riscv_f_ext_state { >> + __u32 f[32]; >> + __u32 fcsr; >> +}; >> + >> +struct __riscv_d_ext_state { >> + __u64 f[32]; >> + __u32 fcsr; >> +}; >> + >> +struct __riscv_q_ext_state { >> + __u64 f[64] __attribute__((aligned(16))); >> + __u32 fcsr; >> + /* Reserved for expansion of sigcontext structure. Currently zeroed >> + * upon signal, and must be zero upon sigreturn. */ >> + __u32 reserved[3]; >> +}; >> + >> +union __riscv_fp_state { >> + struct __riscv_f_ext_state f; >> + struct __riscv_d_ext_state d; >> + struct __riscv_q_ext_state q; >> +}; > > Out of interest, how does one tell which fp format is in use? We might need another tag here -- I'll talk to Andrew (who did the glibc side of this) and make sure we can handle something like running F user code on a D kernel. >> diff --git a/arch/riscv/include/uapi/asm/ucontext.h b/arch/riscv/include/uapi/asm/ucontext.h >> new file mode 100644 >> index 000000000000..52eff9febcfd >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/ucontext.h > ... >> +struct ucontext { >> + unsigned long uc_flags; >> + struct ucontext *uc_link; >> + stack_t uc_stack; >> + sigset_t uc_sigmask; >> + /* glibc uses a 1024-bit sigset_t */ >> + __u8 __unused[1024 / 8 - sizeof(sigset_t)]; >> + /* last for future expansion */ >> + struct sigcontext uc_mcontext; >> +}; > > Any particular reason not to use the asm-generic ucontext? In the generic ucontext, 'uc_sigmask' is at the end of the structure so it can be expanded. Since we want our mcontext to be expandable as well, we pre-allocate some expandable space for sigmask and then put mcontext at the end. We stole this idea from arm64. >> diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h >> new file mode 100644 >> index 000000000000..7e3909ac3c18 >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/unistd.h > ... >> +/* FIXME: This exists for now in order to maintain compatibility with our >> + * pre-upstream glibc, and will be removed for our real Linux submission. >> + */ >> +#define __ARCH_WANT_RENAMEAT >> + > > Don't forget ;-) > > Have you seen the patches floating around for dropping > getrlimit/setrlimit (in favour of prlimit64) and fstatat64/fstat64 (in > favour of statx)? I guess its no big deal. Yes, but we're trying to make this glibc release so we decided to hold off on them. If we can't make it then we might reconsider, but they seem like fairly small issues. >> +#include <asm-generic/unistd.h> >> + >> +/* >> + * These system calls add support for AMOs on RISC-V systems without support >> + * for the A extension. >> + */ >> +#define __NR_sysriscv_cmpxchg32 (__NR_arch_specific_syscall + 0) >> +#define __NR_sysriscv_cmpxchg64 (__NR_arch_specific_syscall + 1) > > I think you need the magic __SYSCALL invocations here like in > include/uapi/asm/unistd.h, otherwise they won't get included in your > syscall table. OK, I've added those. diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h index 7e3909ac3c18..3cdb32912ac7 100644 --- a/arch/riscv/include/uapi/asm/unistd.h +++ b/arch/riscv/include/uapi/asm/unistd.h @@ -23,4 +23,6 @@ * for the A extension. */ #define __NR_sysriscv_cmpxchg32 (__NR_arch_specific_syscall + 0) +__SYSCALL(__NR_sysriscv_cmpxchg32, sys_sysriscv_cmpxchg32) #define __NR_sysriscv_cmpxchg64 (__NR_arch_specific_syscall + 1) +__SYSCALL(__NR_sysriscv_cmpxchg64, sys_sysriscv_cmpxchg64) >> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c >> new file mode 100644 >> index 000000000000..69b3b2d10664 >> --- /dev/null >> +++ b/arch/riscv/kernel/ptrace.c > ... >> +enum riscv_regset { >> + REGSET_X, >> +}; >> + >> +/* >> + * Get registers from task and ready the result for userspace. >> + */ >> +static char *getregs(struct task_struct *child, struct pt_regs *uregs) >> +{ >> + *uregs = *task_pt_regs(child); >> + return (char *)uregs; >> +} >> + >> +/* Put registers back to task. */ >> +static void putregs(struct task_struct *child, struct pt_regs *uregs) >> +{ >> + struct pt_regs *regs = task_pt_regs(child); >> + *regs = *uregs; >> +} >> + >> +static int riscv_gpr_get(struct task_struct *target, >> + const struct user_regset *regset, >> + unsigned int pos, unsigned int count, >> + void *kbuf, void __user *ubuf) >> +{ >> + struct pt_regs regs; >> + >> + getregs(target, ®s); >> + >> + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, ®s, 0, >> + sizeof(regs)); > > Shouldn't this be limited to sizeof(struct user_regs_struct)? > > Why not copy straight out of task_pt_regs(target) instead of bouncing > via the stack? IIRC this code used to be more complicated as it supported the two different ptrace register APIs. There's no reason to have this function now, so I've just pulled into the only caller. >> +} >> + >> +static int riscv_gpr_set(struct task_struct *target, >> + const struct user_regset *regset, >> + unsigned int pos, unsigned int count, >> + const void *kbuf, const void __user *ubuf) >> +{ >> + int ret; >> + struct pt_regs regs; >> + >> + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, ®s, 0, >> + sizeof(regs)); > > likewise. > > In fact if userland supplies insufficient data then this looks > vulnerable to a kernel stack data leak, since regs will remain partially > uninitialised and then get written to the target regs where it can be > read back again. > > If you're going to bounce via the stack I think you need to fully > initialise before using user_regset_copyin, or you could just copy > directly into task_pt_regs(target) for now since, at least for the > current internal struct pt_regs, the begining of pt_regs appears to > match user_regs_struct. > >> + if (ret) >> + return ret; >> + >> + putregs(target, ®s); > > Similarly this needs to be careful not to overwrite the supervisor > registers with whatever was on kernel stack (assuming only partially > copied as suggested above)? > >> + >> + return 0; >> +} >> + >> + >> +static const struct user_regset riscv_user_regset[] = { >> + [REGSET_X] = { >> + .core_note_type = NT_PRSTATUS, >> + .n = ELF_NGREG, >> + .size = sizeof(elf_greg_t), >> + .align = sizeof(elf_greg_t), >> + .get = &riscv_gpr_get, >> + .set = &riscv_gpr_set, >> + }, > > Will the FP registers get exposed at some point as well? > >> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >> new file mode 100644 >> index 000000000000..ab699efe636e >> --- /dev/null >> +++ b/arch/riscv/kernel/sys_riscv.c > ... >> +SYSCALL_DEFINE3(sysriscv_cmpxchg32, unsigned long, arg1, unsigned long, arg2, >> + unsigned long, arg3) >> +{ >> + unsigned long flags; >> + unsigned long prev; > > should that be unsigned int? Else on 64-bit half of it could be left > uninitialised. > >> + unsigned int *ptr; > > should that be tagged with __user? > >> + unsigned int err; >> + >> + ptr = (unsigned int *)arg1; > > I presume you'll need to cast to __user __force to keep sparse happy > here. This should be fixed, I was just lazy when converting from the multiplexed syscall version. > >> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned int))) >> + return -EFAULT; >> + >> + preempt_disable(); >> + raw_local_irq_save(flags); >> + err = __get_user(prev, ptr); >> + if (likely(!err && prev == arg2)) >> + err = __put_user(arg3, ptr); >> + raw_local_irq_restore(flags); >> + preempt_enable(); > > Are user accesses safe from atomic context? What if it needs paging in? > > You could disable page faults but then I think you'd have to handle the > EFAULT again outside of atomic context to try getting it paged in, and > then retry in atomic context. Or perhaps there's a cleaner way that > doesn't come to mind late at night. > > I'm not sure OTOH whether copy on write (i.e. affecting the __put_user() > but not the __get_user() would be problematic. I suppose as long as it > can safely allocate a page it should be fine... Should be possible to > test using madvise(MADV_DONTNEED) (which I think makes pages use the > zero page with copy-on-write). > > Also if this is going to be included on SMP kernels (where I gather > proper atomics are available), does it need an SMP safe version too > which uses proper atomics? On 64-bit machines with the A extension (which is required for SMP) then that's the right thing to do -- we're actually doing it in the VDSO right now, but there's no reason not to do it in the syscall as well. On 32-bit machines, I think it's still not safe as we don't have a 64-bit CAS even with the A extension. I think the best thing to do is actually to disallow the 64-bit CAS on 32-bit machines -- we could disallow this on just SMP machines, but I think it's saner do disallow it everywhere so we don't end up with binaries that won't run on SMP kernels. I'll try to figure out if userspace can work without it, but I think it should be OK as we don't have double-word CAS on 64-bit. >> + >> + return unlikely(err) ? err : prev; >> +} >> + >> +SYSCALL_DEFINE3(sysriscv_cmpxchg64, unsigned long, arg1, unsigned long, arg2, >> + unsigned long, arg3) >> +{ >> + unsigned long flags; >> + unsigned long prev; >> + unsigned int *ptr; > > should that be unsigned long __user *? > >> + unsigned int err; >> + >> + ptr = (unsigned int *)arg1; >> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned long))) >> + return -EFAULT; >> + >> + preempt_disable(); >> + raw_local_irq_save(flags); >> + err = __get_user(prev, ptr); >> + if (likely(!err && prev == arg2)) >> + err = __put_user(arg3, ptr); >> + raw_local_irq_restore(flags); >> + preempt_enable(); > > Likewise to other comments above. > > This doesn't look much different to sysriscv_cmpxchg32 on 32-bit. Is it > meant to be excluded from 32-bit kernels? If so definition of the __NR_ > constant and the __SYSCALL magic in uapi/asm/unistd.h should I presume > be conditional on the ABI. Sorry, that was just a copy-and-paste error. This is intended to actually be a 64-bit CAS on 32-bit machines -- though maybe that was a bad idea.
WARNING: multiple messages have this Message-ID (diff)
From: Palmer Dabbelt <palmer@dabbelt.com> To: james.hogan@imgtec.com Cc: peterz@infradead.org, mingo@redhat.com, mcgrof@kernel.org, viro@zeniv.linux.org.uk, sfr@canb.auug.org.au, nicolas.dichtel@6wind.com, rmk+kernel@armlinux.org.uk, msalter@redhat.com, tklauser@distanz.ch, will.deacon@arm.com, paul.gortmaker@windriver.com, linux@roeck-us.net, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, albert@sifive.com Subject: Re: [PATCH 8/9] RISC-V: User-facing API Date: Thu, 29 Jun 2017 14:42:38 -0700 (PDT) [thread overview] Message-ID: <mhng-21eaeea1-4cd1-43b1-9b2b-b664cea12d43@palmer-si-x1c4> (raw) In-Reply-To: <20170628224237.GO6973@jhogan-linux.le.imgtec.org> On Wed, 28 Jun 2017 15:42:37 PDT (-0700), james.hogan@imgtec.com wrote: > Hi Palmer, > > On Wed, Jun 28, 2017 at 11:55:37AM -0700, Palmer Dabbelt wrote: >> diff --git a/arch/riscv/include/asm/syscalls.h b/arch/riscv/include/asm/syscalls.h >> new file mode 100644 >> index 000000000000..d85267c4f7ea >> --- /dev/null >> +++ b/arch/riscv/include/asm/syscalls.h >> @@ -0,0 +1,25 @@ > ... >> +/* kernel/sys_riscv.c */ >> +asmlinkage long sys_sysriscv(unsigned long, unsigned long, >> + unsigned long, unsigned long); > > You suggested in the cover letter this wasn't muxed any longer, maybe > you should have a prototype for each of the cmpxchg syscalls instead? Sorry, I just missed that. I'll fix it for the v4 diff --git a/arch/riscv/include/asm/syscalls.h b/arch/riscv/include/asm/syscalls.h index d85267c4f7ea..6490274fbb76 100644 --- a/arch/riscv/include/asm/syscalls.h +++ b/arch/riscv/include/asm/syscalls.h @@ -19,7 +19,7 @@ #include <asm-generic/syscalls.h> /* kernel/sys_riscv.c */ -asmlinkage long sys_sysriscv(unsigned long, unsigned long, - unsigned long, unsigned long); +asmlinkage long sys_sysriscv_cmpxchg32(u32 __user * ptr, u32 new, u32 old); +asmlinkage long sys_sysriscv_cmpxchg64(u64 __user * ptr, u64 new, u64 old); #endif /* _ASM_RISCV_SYSCALLS_H */ >> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h >> new file mode 100644 >> index 000000000000..01aee1654eae >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/ptrace.h > ... >> +struct __riscv_f_ext_state { >> + __u32 f[32]; >> + __u32 fcsr; >> +}; >> + >> +struct __riscv_d_ext_state { >> + __u64 f[32]; >> + __u32 fcsr; >> +}; >> + >> +struct __riscv_q_ext_state { >> + __u64 f[64] __attribute__((aligned(16))); >> + __u32 fcsr; >> + /* Reserved for expansion of sigcontext structure. Currently zeroed >> + * upon signal, and must be zero upon sigreturn. */ >> + __u32 reserved[3]; >> +}; >> + >> +union __riscv_fp_state { >> + struct __riscv_f_ext_state f; >> + struct __riscv_d_ext_state d; >> + struct __riscv_q_ext_state q; >> +}; > > Out of interest, how does one tell which fp format is in use? We might need another tag here -- I'll talk to Andrew (who did the glibc side of this) and make sure we can handle something like running F user code on a D kernel. >> diff --git a/arch/riscv/include/uapi/asm/ucontext.h b/arch/riscv/include/uapi/asm/ucontext.h >> new file mode 100644 >> index 000000000000..52eff9febcfd >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/ucontext.h > ... >> +struct ucontext { >> + unsigned long uc_flags; >> + struct ucontext *uc_link; >> + stack_t uc_stack; >> + sigset_t uc_sigmask; >> + /* glibc uses a 1024-bit sigset_t */ >> + __u8 __unused[1024 / 8 - sizeof(sigset_t)]; >> + /* last for future expansion */ >> + struct sigcontext uc_mcontext; >> +}; > > Any particular reason not to use the asm-generic ucontext? In the generic ucontext, 'uc_sigmask' is at the end of the structure so it can be expanded. Since we want our mcontext to be expandable as well, we pre-allocate some expandable space for sigmask and then put mcontext at the end. We stole this idea from arm64. >> diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h >> new file mode 100644 >> index 000000000000..7e3909ac3c18 >> --- /dev/null >> +++ b/arch/riscv/include/uapi/asm/unistd.h > ... >> +/* FIXME: This exists for now in order to maintain compatibility with our >> + * pre-upstream glibc, and will be removed for our real Linux submission. >> + */ >> +#define __ARCH_WANT_RENAMEAT >> + > > Don't forget ;-) > > Have you seen the patches floating around for dropping > getrlimit/setrlimit (in favour of prlimit64) and fstatat64/fstat64 (in > favour of statx)? I guess its no big deal. Yes, but we're trying to make this glibc release so we decided to hold off on them. If we can't make it then we might reconsider, but they seem like fairly small issues. >> +#include <asm-generic/unistd.h> >> + >> +/* >> + * These system calls add support for AMOs on RISC-V systems without support >> + * for the A extension. >> + */ >> +#define __NR_sysriscv_cmpxchg32 (__NR_arch_specific_syscall + 0) >> +#define __NR_sysriscv_cmpxchg64 (__NR_arch_specific_syscall + 1) > > I think you need the magic __SYSCALL invocations here like in > include/uapi/asm/unistd.h, otherwise they won't get included in your > syscall table. OK, I've added those. diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h index 7e3909ac3c18..3cdb32912ac7 100644 --- a/arch/riscv/include/uapi/asm/unistd.h +++ b/arch/riscv/include/uapi/asm/unistd.h @@ -23,4 +23,6 @@ * for the A extension. */ #define __NR_sysriscv_cmpxchg32 (__NR_arch_specific_syscall + 0) +__SYSCALL(__NR_sysriscv_cmpxchg32, sys_sysriscv_cmpxchg32) #define __NR_sysriscv_cmpxchg64 (__NR_arch_specific_syscall + 1) +__SYSCALL(__NR_sysriscv_cmpxchg64, sys_sysriscv_cmpxchg64) >> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c >> new file mode 100644 >> index 000000000000..69b3b2d10664 >> --- /dev/null >> +++ b/arch/riscv/kernel/ptrace.c > ... >> +enum riscv_regset { >> + REGSET_X, >> +}; >> + >> +/* >> + * Get registers from task and ready the result for userspace. >> + */ >> +static char *getregs(struct task_struct *child, struct pt_regs *uregs) >> +{ >> + *uregs = *task_pt_regs(child); >> + return (char *)uregs; >> +} >> + >> +/* Put registers back to task. */ >> +static void putregs(struct task_struct *child, struct pt_regs *uregs) >> +{ >> + struct pt_regs *regs = task_pt_regs(child); >> + *regs = *uregs; >> +} >> + >> +static int riscv_gpr_get(struct task_struct *target, >> + const struct user_regset *regset, >> + unsigned int pos, unsigned int count, >> + void *kbuf, void __user *ubuf) >> +{ >> + struct pt_regs regs; >> + >> + getregs(target, ®s); >> + >> + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, ®s, 0, >> + sizeof(regs)); > > Shouldn't this be limited to sizeof(struct user_regs_struct)? > > Why not copy straight out of task_pt_regs(target) instead of bouncing > via the stack? IIRC this code used to be more complicated as it supported the two different ptrace register APIs. There's no reason to have this function now, so I've just pulled into the only caller. >> +} >> + >> +static int riscv_gpr_set(struct task_struct *target, >> + const struct user_regset *regset, >> + unsigned int pos, unsigned int count, >> + const void *kbuf, const void __user *ubuf) >> +{ >> + int ret; >> + struct pt_regs regs; >> + >> + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, ®s, 0, >> + sizeof(regs)); > > likewise. > > In fact if userland supplies insufficient data then this looks > vulnerable to a kernel stack data leak, since regs will remain partially > uninitialised and then get written to the target regs where it can be > read back again. > > If you're going to bounce via the stack I think you need to fully > initialise before using user_regset_copyin, or you could just copy > directly into task_pt_regs(target) for now since, at least for the > current internal struct pt_regs, the begining of pt_regs appears to > match user_regs_struct. > >> + if (ret) >> + return ret; >> + >> + putregs(target, ®s); > > Similarly this needs to be careful not to overwrite the supervisor > registers with whatever was on kernel stack (assuming only partially > copied as suggested above)? > >> + >> + return 0; >> +} >> + >> + >> +static const struct user_regset riscv_user_regset[] = { >> + [REGSET_X] = { >> + .core_note_type = NT_PRSTATUS, >> + .n = ELF_NGREG, >> + .size = sizeof(elf_greg_t), >> + .align = sizeof(elf_greg_t), >> + .get = &riscv_gpr_get, >> + .set = &riscv_gpr_set, >> + }, > > Will the FP registers get exposed at some point as well? > >> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c >> new file mode 100644 >> index 000000000000..ab699efe636e >> --- /dev/null >> +++ b/arch/riscv/kernel/sys_riscv.c > ... >> +SYSCALL_DEFINE3(sysriscv_cmpxchg32, unsigned long, arg1, unsigned long, arg2, >> + unsigned long, arg3) >> +{ >> + unsigned long flags; >> + unsigned long prev; > > should that be unsigned int? Else on 64-bit half of it could be left > uninitialised. > >> + unsigned int *ptr; > > should that be tagged with __user? > >> + unsigned int err; >> + >> + ptr = (unsigned int *)arg1; > > I presume you'll need to cast to __user __force to keep sparse happy > here. This should be fixed, I was just lazy when converting from the multiplexed syscall version. > >> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned int))) >> + return -EFAULT; >> + >> + preempt_disable(); >> + raw_local_irq_save(flags); >> + err = __get_user(prev, ptr); >> + if (likely(!err && prev == arg2)) >> + err = __put_user(arg3, ptr); >> + raw_local_irq_restore(flags); >> + preempt_enable(); > > Are user accesses safe from atomic context? What if it needs paging in? > > You could disable page faults but then I think you'd have to handle the > EFAULT again outside of atomic context to try getting it paged in, and > then retry in atomic context. Or perhaps there's a cleaner way that > doesn't come to mind late at night. > > I'm not sure OTOH whether copy on write (i.e. affecting the __put_user() > but not the __get_user() would be problematic. I suppose as long as it > can safely allocate a page it should be fine... Should be possible to > test using madvise(MADV_DONTNEED) (which I think makes pages use the > zero page with copy-on-write). > > Also if this is going to be included on SMP kernels (where I gather > proper atomics are available), does it need an SMP safe version too > which uses proper atomics? On 64-bit machines with the A extension (which is required for SMP) then that's the right thing to do -- we're actually doing it in the VDSO right now, but there's no reason not to do it in the syscall as well. On 32-bit machines, I think it's still not safe as we don't have a 64-bit CAS even with the A extension. I think the best thing to do is actually to disallow the 64-bit CAS on 32-bit machines -- we could disallow this on just SMP machines, but I think it's saner do disallow it everywhere so we don't end up with binaries that won't run on SMP kernels. I'll try to figure out if userspace can work without it, but I think it should be OK as we don't have double-word CAS on 64-bit. >> + >> + return unlikely(err) ? err : prev; >> +} >> + >> +SYSCALL_DEFINE3(sysriscv_cmpxchg64, unsigned long, arg1, unsigned long, arg2, >> + unsigned long, arg3) >> +{ >> + unsigned long flags; >> + unsigned long prev; >> + unsigned int *ptr; > > should that be unsigned long __user *? > >> + unsigned int err; >> + >> + ptr = (unsigned int *)arg1; >> + if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned long))) >> + return -EFAULT; >> + >> + preempt_disable(); >> + raw_local_irq_save(flags); >> + err = __get_user(prev, ptr); >> + if (likely(!err && prev == arg2)) >> + err = __put_user(arg3, ptr); >> + raw_local_irq_restore(flags); >> + preempt_enable(); > > Likewise to other comments above. > > This doesn't look much different to sysriscv_cmpxchg32 on 32-bit. Is it > meant to be excluded from 32-bit kernels? If so definition of the __NR_ > constant and the __SYSCALL magic in uapi/asm/unistd.h should I presume > be conditional on the ABI. Sorry, that was just a copy-and-paste error. This is intended to actually be a 64-bit CAS on 32-bit machines -- though maybe that was a bad idea.
next prev parent reply other threads:[~2017-06-29 21:42 UTC|newest] Thread overview: 297+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-23 0:41 RISC-V Linux Port v1 Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 1/7] RISC-V: Top-Level Makefile for riscv{32,64} Palmer Dabbelt 2017-05-23 11:30 ` Arnd Bergmann 2017-05-27 0:57 ` Palmer Dabbelt 2017-05-29 10:50 ` Arnd Bergmann 2017-06-06 4:56 ` Palmer Dabbelt 2017-06-06 17:39 ` Karsten Merker 2017-06-06 17:57 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 2/7] RISC-V: arch/riscv Makefile and Kconfigs Palmer Dabbelt 2017-05-23 1:27 ` Olof Johansson 2017-05-23 1:31 ` Randy Dunlap 2017-05-23 4:49 ` Palmer Dabbelt 2017-05-23 4:49 ` Palmer Dabbelt 2017-05-23 5:16 ` [patches] " Olof Johansson 2017-05-23 21:07 ` Benjamin Herrenschmidt 2017-05-23 5:23 ` Olof Johansson 2017-05-23 15:29 ` Palmer Dabbelt 2017-05-23 10:51 ` Geert Uytterhoeven 2017-05-25 1:59 ` Palmer Dabbelt 2017-05-23 11:46 ` Arnd Bergmann 2017-05-27 0:57 ` Palmer Dabbelt 2017-05-29 11:17 ` Arnd Bergmann 2017-06-06 4:56 ` Palmer Dabbelt 2017-06-06 9:20 ` Arnd Bergmann 2017-06-06 20:38 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 3/7] RISC-V: Device Tree Documentation Palmer Dabbelt 2017-05-23 12:03 ` Arnd Bergmann 2017-05-27 0:57 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 4/7] RISC-V: arch/riscv/include Palmer Dabbelt 2017-05-23 12:55 ` Arnd Bergmann 2017-05-23 21:23 ` Benjamin Herrenschmidt 2017-06-03 2:00 ` Palmer Dabbelt 2017-06-01 0:56 ` Palmer Dabbelt 2017-06-01 9:00 ` Arnd Bergmann 2017-06-06 4:56 ` Palmer Dabbelt 2017-06-06 8:54 ` Arnd Bergmann 2017-06-06 19:07 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 5/7] RISC-V: arch/riscv/lib Palmer Dabbelt 2017-05-23 10:47 ` Geert Uytterhoeven 2017-05-23 22:07 ` Palmer Dabbelt 2017-05-23 11:19 ` Arnd Bergmann 2017-05-25 1:59 ` Palmer Dabbelt 2017-05-26 9:06 ` Arnd Bergmann 2017-06-06 4:56 ` Palmer Dabbelt 2017-06-06 9:31 ` Arnd Bergmann 2017-06-06 20:53 ` Palmer Dabbelt 2017-06-07 7:35 ` Arnd Bergmann 2017-06-23 23:24 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 6/7] RISC-V: arch/riscv/kernel Palmer Dabbelt 2017-05-23 2:11 ` Olof Johansson 2017-05-25 1:59 ` Palmer Dabbelt 2017-05-25 19:51 ` Arnd Bergmann 2017-06-06 4:56 ` Palmer Dabbelt 2017-06-06 9:03 ` Arnd Bergmann 2017-06-06 20:38 ` Palmer Dabbelt 2017-05-23 13:35 ` Arnd Bergmann 2017-06-02 23:56 ` Palmer Dabbelt 2017-06-06 9:01 ` Arnd Bergmann 2017-06-06 20:37 ` Palmer Dabbelt 2017-05-25 17:05 ` Pavel Machek 2017-06-03 3:32 ` Palmer Dabbelt 2017-05-23 0:41 ` [PATCH 7/7] RISC-V: arch/riscv/mm Palmer Dabbelt 2017-05-23 1:28 ` Randy Dunlap 2017-05-23 2:17 ` Olof Johansson 2017-05-23 3:36 ` Palmer Dabbelt 2017-05-23 1:16 ` RISC-V Linux Port v1 Olof Johansson 2017-05-23 1:25 ` Randy Dunlap 2017-05-23 3:36 ` Palmer Dabbelt 2017-05-23 3:36 ` Palmer Dabbelt 2017-05-23 6:45 ` Tobias Klauser 2017-05-23 15:44 ` Palmer Dabbelt 2017-05-23 2:16 ` Randy Dunlap 2017-05-23 4:49 ` Palmer Dabbelt 2017-06-06 22:59 ` RISC-V Linux Port v2 Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 01/17] drivers: support PCIe in RISCV Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:17 ` Geert Uytterhoeven 2017-06-07 14:25 ` Christoph Hellwig [not found] ` <CAMgXwTjXZ5dsxmJ2FyWhCRWo-3nyvKUDfhfV0nNC+oakF=AEsA@mail.gmail.com> 2017-06-07 17:40 ` Olof Johansson 2017-06-23 21:47 ` [patches] " Palmer Dabbelt 2017-06-23 21:47 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 02/17] pcie-xilinx: add missing 5th legacy interrupt Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:18 ` Geert Uytterhoeven 2017-06-07 9:24 ` Marc Zyngier 2017-06-07 19:03 ` Wesley Terpstra 2017-06-06 22:59 ` [PATCH 03/17] base: fix order of OF initialization Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:07 ` Geert Uytterhoeven 2017-06-07 9:35 ` Mark Rutland 2017-06-07 9:35 ` Mark Rutland 2017-06-07 18:39 ` Wesley Terpstra 2017-06-07 21:10 ` Benjamin Herrenschmidt 2017-06-08 3:49 ` Frank Rowand 2017-06-08 9:05 ` Mark Rutland 2017-06-08 9:05 ` Mark Rutland 2017-06-08 9:05 ` Mark Rutland 2017-06-09 0:37 ` Frank Rowand 2017-06-06 22:59 ` [PATCH 04/17] Documentation: atomic_ops.txt is core-api/atomic_ops.rst Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:19 ` Geert Uytterhoeven 2017-06-07 9:20 ` Will Deacon 2017-06-06 22:59 ` [PATCH 05/17] MAINTAINERS: Add RISC-V Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 06/17] pci: Add generic pcibios_{fixup_bus,align_resource} Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:19 ` Geert Uytterhoeven 2017-06-07 8:01 ` Arnd Bergmann 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-08 8:12 ` Christoph Hellwig 2017-06-08 8:35 ` Arnd Bergmann 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 07/17] lib: Add shared copies of some GCC library routines Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:11 ` Geert Uytterhoeven 2017-06-07 10:13 ` Mark Rutland 2017-06-07 18:57 ` Wesley Terpstra 2017-06-07 19:57 ` Rob Herring 2017-06-07 20:31 ` Wesley Terpstra 2017-06-08 10:52 ` Mark Rutland 2017-06-09 21:46 ` Wesley Terpstra 2017-06-09 21:46 ` Wesley Terpstra 2017-06-09 21:58 ` Wesley Terpstra 2017-06-19 14:30 ` Mark Rutland 2017-06-07 22:27 ` Luis R. Rodriguez 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-06 22:59 ` [PATCH 09/17] clocksource/timer-riscv: New RISC-V Clocksource Palmer Dabbelt 2017-06-06 22:59 ` Palmer Dabbelt 2017-06-07 7:12 ` Geert Uytterhoeven 2017-06-07 7:25 ` Arnd Bergmann 2017-06-23 23:24 ` Palmer Dabbelt 2017-06-23 23:24 ` Palmer Dabbelt 2017-06-07 9:43 ` Marc Zyngier 2017-06-24 2:02 ` Palmer Dabbelt 2017-06-24 2:02 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 10/17] irqchip: New RISC-V PLIC Driver Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-07 7:13 ` Geert Uytterhoeven 2017-06-07 7:55 ` Arnd Bergmann 2017-06-24 0:45 ` Palmer Dabbelt 2017-06-24 0:45 ` Palmer Dabbelt 2017-06-07 10:52 ` Marc Zyngier 2017-06-09 13:47 ` Will Deacon 2017-06-27 1:09 ` Palmer Dabbelt 2017-06-27 1:09 ` Palmer Dabbelt 2017-06-25 20:49 ` Palmer Dabbelt 2017-06-25 20:49 ` Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 11/17] irqchip: RISC-V Local Interrupt Controller Driver Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-07 7:14 ` Geert Uytterhoeven 2017-06-06 23:00 ` [PATCH 12/17] tty: New RISC-V SBI Console Driver Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-07 7:15 ` Geert Uytterhoeven 2017-06-07 7:58 ` Arnd Bergmann 2017-06-24 0:45 ` Palmer Dabbelt 2017-06-24 0:45 ` Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 13/17] RISC-V: Add include subdirectory Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-07 8:12 ` Arnd Bergmann 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-24 2:01 ` Palmer Dabbelt 2017-06-24 15:42 ` Benjamin Herrenschmidt 2017-06-24 21:32 ` [patches] " Palmer Dabbelt 2017-06-24 21:32 ` Palmer Dabbelt 2017-06-25 3:01 ` Benjamin Herrenschmidt 2017-06-07 11:54 ` Peter Zijlstra 2017-06-07 12:25 ` Peter Zijlstra 2017-06-07 12:06 ` Peter Zijlstra 2017-06-07 12:18 ` Peter Zijlstra 2017-06-07 12:36 ` Peter Zijlstra 2017-06-07 12:58 ` Peter Zijlstra 2017-06-07 13:16 ` Will Deacon 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-27 0:07 ` Daniel Lustig 2017-06-27 8:48 ` Will Deacon 2017-06-07 16:35 ` Peter Zijlstra 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-07 12:42 ` Peter Zijlstra 2017-06-07 13:17 ` Peter Zijlstra 2017-06-09 8:16 ` Peter Zijlstra 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-26 20:07 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 14/17] RISC-V: lib files Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 15/17] RISC-V: Add mm subdirectory Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 16/17] RISC-V: Add kernel subdirectory Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` [PATCH 17/17] RISC-V: Makefile and Kconfig Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-06 23:00 ` Palmer Dabbelt 2017-06-07 9:23 ` RISC-V Linux Port v2 Will Deacon 2017-06-07 21:54 ` Palmer Dabbelt 2017-06-07 21:54 ` Palmer Dabbelt 2017-06-08 10:26 ` Will Deacon 2017-06-08 18:16 ` Palmer Dabbelt 2017-06-08 18:16 ` Palmer Dabbelt 2017-06-28 18:55 ` RISC-V Linux Port v3 Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 1/9] RISC-V: Init and Halt Code Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-29 9:44 ` Geert Uytterhoeven 2017-06-29 9:44 ` Geert Uytterhoeven 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 3/9] RISC-V: Generic library routines and assembly Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 4/9] RISC-V: ELF and module implementation Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 5/9] RISC-V: Task implementation Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 23:32 ` James Hogan 2017-06-28 23:32 ` James Hogan 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-29 8:22 ` Tobias Klauser 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 6/9] RISC-V: Device, timer, IRQs, and the SBI Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-29 8:39 ` Tobias Klauser 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-29 22:52 ` Palmer Dabbelt 2017-06-30 7:57 ` Tobias Klauser 2017-06-28 18:55 ` [PATCH 7/9] RISC-V: Paging and MMU Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 23:09 ` James Hogan 2017-06-28 23:09 ` James Hogan 2017-06-29 22:11 ` Palmer Dabbelt 2017-06-29 22:11 ` Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 21:49 ` Thomas Gleixner 2017-06-28 21:52 ` Thomas Gleixner 2017-06-29 17:22 ` Palmer Dabbelt 2017-06-29 17:22 ` Palmer Dabbelt 2017-06-28 22:42 ` James Hogan 2017-06-28 22:42 ` James Hogan 2017-06-29 21:42 ` Palmer Dabbelt [this message] 2017-06-29 21:42 ` Palmer Dabbelt 2017-07-03 23:06 ` James Hogan 2017-07-03 23:06 ` James Hogan 2017-07-05 16:49 ` Palmer Dabbelt 2017-07-05 16:49 ` Palmer Dabbelt 2017-06-28 18:55 ` [PATCH 9/9] RISC-V: Build Infastructure Palmer Dabbelt 2017-06-28 18:55 ` Palmer Dabbelt 2017-06-28 21:05 ` Karsten Merker 2017-06-28 21:13 ` Palmer Dabbelt 2017-06-28 21:13 ` Palmer Dabbelt 2017-06-28 21:25 ` James Hogan 2017-06-28 21:25 ` James Hogan 2017-06-29 16:29 ` Palmer Dabbelt 2017-06-29 16:29 ` Palmer Dabbelt 2017-06-28 22:54 ` James Hogan 2017-06-28 22:54 ` James Hogan 2017-06-29 22:11 ` Palmer Dabbelt 2017-06-29 22:11 ` Palmer Dabbelt 2017-06-06 22:59 ` RISC-V Linux Port v2 Palmer Dabbelt 2017-06-07 7:29 ` David Howells 2017-06-07 21:54 ` Palmer Dabbelt 2017-06-07 21:54 ` Palmer Dabbelt 2017-06-07 21:54 ` Palmer Dabbelt 2017-07-04 19:50 RISC-V Linux Port v4 Palmer Dabbelt 2017-07-04 19:51 ` [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt 2017-07-04 19:51 ` Palmer Dabbelt 2017-07-04 19:51 ` Palmer Dabbelt 2017-07-05 10:24 ` James Hogan 2017-07-05 10:24 ` James Hogan 2017-07-06 2:01 ` Christoph Hellwig 2017-07-06 8:55 ` Will Deacon 2017-07-06 15:34 ` Christoph Hellwig 2017-07-06 15:45 ` Will Deacon [not found] ` <mhng-f92ef7c4-049a-4a71-be12-c600d1d7858b@palmer-si-x1c4> 2017-07-10 20:18 ` Palmer Dabbelt 2017-07-11 13:22 ` Will Deacon 2017-07-11 13:55 ` Christoph Hellwig 2017-07-11 17:28 ` Palmer Dabbelt 2017-07-11 17:28 ` Palmer Dabbelt 2017-07-11 17:07 ` Palmer Dabbelt 2017-07-06 15:34 ` Dave P Martin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=mhng-21eaeea1-4cd1-43b1-9b2b-b664cea12d43@palmer-si-x1c4 \ --to=palmer@dabbelt.com \ --cc=james.hogan@imgtec.com \ --cc=peterz@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.