On 2/10/22 11:26, John David Anglin wrote:
> On 2022-02-09 1:24 p.m., Helge Deller wrote:
>> After commit 4b9d2a731c3d ("parisc: Switch user access functions
>> to signal errors in r29 instead of r8") bash suddenly started
>> to report those warnings after login:
>>
>> -bash: cannot set terminal process group (-1): Bad file descriptor
>> -bash: no job control in this shell
>>
>> It turned out, that another function call inside a put_user(), e.g.:
>> put_user(vt_do_kdgkbmode(console), (int __user *)arg);
>> clobbered the error register (r29) and thus failed.
>> Avoid this miscompilation by first calculate the value in
>> __put_user() before calling __put_user_internal() to actually
>> write the value to user space.
> Couldn't the same issue occur with ptr argument in these macros?
Theoretically, yes. Haven't seen it though.
>> Additionally, prefer the "+" constraint on pu_err and gu_err registers to tell
>> the compiler that those operands are both read and written by the assembly
>> instruction.
>>
>> Signed-off-by: Helge Deller <deller@xxxxxx>
>> Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8")
>>
>> diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
>> index ebf8a845b017..68f5c1eaaa6f 100644
>> --- a/arch/parisc/include/asm/uaccess.h
>> +++ b/arch/parisc/include/asm/uaccess.h
>> @@ -89,7 +89,7 @@ struct exception_table_entry {
>> __asm__("1: " ldx " 0(" sr "%2),%0\n" \
>> "9:\n" \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \
>> - : "=r"(__gu_val), "=r"(__gu_err) \
>> + : "=&r"(__gu_val), "+r"(__gu_err) \
> I don't believe the early clobber is needed on __gnu_val.
Right.
>> : "r"(ptr), "1"(__gu_err)); \
>> \
>> (val) = (__force __typeof__(*(ptr))) __gu_val; \
>> @@ -123,7 +123,7 @@ struct exception_table_entry {
>> "9:\n" \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \
>> - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \
>> + : "=&r"(__gu_tmp.l), "+r"(__gu_err) \
>> : "r"(ptr), "1"(__gu_err)); \
>> \
>> (val) = __gu_tmp.t; \
>> @@ -132,10 +132,9 @@ struct exception_table_entry {
>> #endif /* !defined(CONFIG_64BIT) */
>>
>>
>> -#define __put_user_internal(sr, x, ptr) \
>> +#define __put_user_internal(sr, __x, ptr) \
>> ({ \
>> ASM_EXCEPTIONTABLE_VAR(__pu_err); \
>> - __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \
>> \
>> switch (sizeof(*(ptr))) { \
>> case 1: __put_user_asm(sr, "stb", __x, ptr); break; \
>> @@ -150,7 +149,9 @@ struct exception_table_entry {
>>
>> #define __put_user(x, ptr) \
>> ({ \
>> - __put_user_internal("%%sr3,", x, ptr); \
>> + __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \
> Whitespace?
Will fix.
>> + \
>> + __put_user_internal("%%sr3,", __x, ptr); \
>> })
>>
>> #define __put_kernel_nofault(dst, src, type, err_label) \
>> @@ -180,7 +181,7 @@ struct exception_table_entry {
>> "1: " stx " %2,0(" sr "%1)\n" \
>> "9:\n" \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \
>> - : "=r"(__pu_err) \
>> + : "+r"(__pu_err) \
>> : "r"(ptr), "r"(x), "0"(__pu_err))
>>
>>
>> @@ -193,7 +194,7 @@ struct exception_table_entry {
>> "9:\n" \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \
>> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \
>> - : "=r"(__pu_err) \
>> + : "+r"(__pu_err) \
>> : "r"(ptr), "r"(__val), "0"(__pu_err)); \
>> } while (0)
Overall, this current patch turned out to not cover all cases.
I think it's too complicated to really try prevent the compiler to
optimize it here, so I'll send out a new patch shortly which
simply initializes __gu_err and __pu_err in the assembly statement itself.
My other codings to avoid that would increase the code a lot,
and I'm not sure if it's even then possible to avoid the failure always.
Helge
