Re: [PATCH v1 03/15] powerpc/uaccess: Remove __get/put_user_inatomic()
From: Daniel Axtens <hidden>
Date: 2021-03-02 07:32:04
Also in:
lkml
Christophe Leroy [off-list ref] writes:
Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
pagefault_disable()"), __get/put_user() can be used in atomic parts
of the code, therefore the __get/put_user_inatomic() introduced
by commit e68c825bb016 ("[POWERPC] Add inatomic versions of __get_user
and __put_user") have become useless.I spent some time chasing these macro definitions. Let me see if I understand you. __get_user(x, ptr) becomes __get_user_nocheck(..., true) __get_user_inatomic() become __get_user_nosleep() The difference between how __get_user_nosleep() and __get_user_nocheck(..., true) operate is that __get_user_nocheck calls might_fault() and __get_user_nosleep() does not. If I understand the commit you reference and mm/memory.c, you're saying that we can indeed call might_fault() when page faults are disabled, because __might_fault() checks if page faults are disabled and does not fire a warning if it is called with page faults disabled. Therefore, it is safe to remove our _inatomic version that does not call might_fault and just to call might_fault unconditionally. Is that right? I haven't checked changes you made to the various .c files in fine detail but they appear to be entirely mechanical.
powerpc is the only one having such functions. There is a real
intention not to have to provide such _inatomic() helpers, see the
comment in might_fault() in mm/memory.c introduced by
commit 3ee1afa308f2 ("x86: some lock annotations for user
copy paths, v2"):
/*
* it would be nicer only to annotate paths which are not under
* pagefault_disable, however that requires a larger audit and
* providing helpers like get_user_atomic.
*/I'm not fully sure I understand what you're saying in this part of the commit message. Kind regards, Daniel
quoted hunk ↗ jump to hunk
Signed-off-by: Christophe Leroy <redacted> --- arch/powerpc/include/asm/uaccess.h | 37 ------------------- arch/powerpc/kernel/align.c | 32 ++++++++-------- .../kernel/hw_breakpoint_constraints.c | 2 +- arch/powerpc/kernel/traps.c | 2 +- 4 files changed, 18 insertions(+), 55 deletions(-)diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h index a08c482b1315..01aea0df4dd0 100644 --- a/arch/powerpc/include/asm/uaccess.h +++ b/arch/powerpc/include/asm/uaccess.h@@ -53,11 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size) #define __put_user(x, ptr) \ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr))) -#define __get_user_inatomic(x, ptr) \ - __get_user_nosleep((x), (ptr), sizeof(*(ptr))) -#define __put_user_inatomic(x, ptr) \ - __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr))) - #ifdef CONFIG_PPC64 #define ___get_user_instr(gu_op, dest, ptr) \@@ -92,9 +87,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size) #define __get_user_instr(x, ptr) \ ___get_user_instr(__get_user, x, ptr) -#define __get_user_instr_inatomic(x, ptr) \ - ___get_user_instr(__get_user_inatomic, x, ptr) - extern long __put_user_bad(void); #define __put_user_size(x, ptr, size, retval) \@@ -141,20 +133,6 @@ __pu_failed: \ __pu_err; \ }) -#define __put_user_nosleep(x, ptr, size) \ -({ \ - long __pu_err; \ - __typeof__(*(ptr)) __user *__pu_addr = (ptr); \ - __typeof__(*(ptr)) __pu_val = (x); \ - __typeof__(size) __pu_size = (size); \ - \ - __chk_user_ptr(__pu_addr); \ - __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \ - \ - __pu_err; \ -}) - - /* * We don't tell gcc that we are accessing memory, but this is OK * because we do not write to any memory gcc knows about, so there@@ -320,21 +298,6 @@ do { \ __gu_err; \ }) -#define __get_user_nosleep(x, ptr, size) \ -({ \ - long __gu_err; \ - __long_type(*(ptr)) __gu_val; \ - __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ - __typeof__(size) __gu_size = (size); \ - \ - __chk_user_ptr(__gu_addr); \ - __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \ - (x) = (__force __typeof__(*(ptr)))__gu_val; \ - \ - __gu_err; \ -}) - - /* more complex routines */ extern unsigned long __copy_tofrom_user(void __user *to,diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c index c7797eb958c7..83b199026a1e 100644 --- a/arch/powerpc/kernel/align.c +++ b/arch/powerpc/kernel/align.c@@ -174,18 +174,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg, switch (nb) { case 8: - ret |= __get_user_inatomic(temp.v[0], p++); - ret |= __get_user_inatomic(temp.v[1], p++); - ret |= __get_user_inatomic(temp.v[2], p++); - ret |= __get_user_inatomic(temp.v[3], p++); + ret |= __get_user(temp.v[0], p++); + ret |= __get_user(temp.v[1], p++); + ret |= __get_user(temp.v[2], p++); + ret |= __get_user(temp.v[3], p++); fallthrough; case 4: - ret |= __get_user_inatomic(temp.v[4], p++); - ret |= __get_user_inatomic(temp.v[5], p++); + ret |= __get_user(temp.v[4], p++); + ret |= __get_user(temp.v[5], p++); fallthrough; case 2: - ret |= __get_user_inatomic(temp.v[6], p++); - ret |= __get_user_inatomic(temp.v[7], p++); + ret |= __get_user(temp.v[6], p++); + ret |= __get_user(temp.v[7], p++); if (unlikely(ret)) return -EFAULT; }@@ -259,18 +259,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg, p = addr; switch (nb) { case 8: - ret |= __put_user_inatomic(data.v[0], p++); - ret |= __put_user_inatomic(data.v[1], p++); - ret |= __put_user_inatomic(data.v[2], p++); - ret |= __put_user_inatomic(data.v[3], p++); + ret |= __put_user(data.v[0], p++); + ret |= __put_user(data.v[1], p++); + ret |= __put_user(data.v[2], p++); + ret |= __put_user(data.v[3], p++); fallthrough; case 4: - ret |= __put_user_inatomic(data.v[4], p++); - ret |= __put_user_inatomic(data.v[5], p++); + ret |= __put_user(data.v[4], p++); + ret |= __put_user(data.v[5], p++); fallthrough; case 2: - ret |= __put_user_inatomic(data.v[6], p++); - ret |= __put_user_inatomic(data.v[7], p++); + ret |= __put_user(data.v[6], p++); + ret |= __put_user(data.v[7], p++); } if (unlikely(ret)) return -EFAULT;diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c index 867ee4aa026a..675d1f66ab72 100644 --- a/arch/powerpc/kernel/hw_breakpoint_constraints.c +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c@@ -141,7 +141,7 @@ void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr, { struct instruction_op op; - if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip)) + if (__get_user_instr(*instr, (void __user *)regs->nip)) return; analyse_instr(&op, regs, *instr);diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 1583fd1c6010..1fa36bd08efe 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c@@ -864,7 +864,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs) unsigned long ea, msr, msr_mask; bool swap; - if (__get_user_inatomic(instr, (unsigned int __user *)regs->nip)) + if (__get_user(instr, (unsigned int __user *)regs->nip)) return; /*-- 2.25.0