Hi Christophe,
> Commit 6bfd93c32a50 ("powerpc: Fix incorrect might_sleep in
> __get_user/__put_user on kernel addresses") added a check to not call
> might_sleep() on kernel addresses. This was to enable the use of
> __get_user() in the alignment exception handler for any address.
>
> Then commit 95156f0051cb ("lockdep, mm: fix might_fault() annotation")
> added a check of the address space in might_fault(), based on
> set_fs() logic. But this didn't solve the powerpc alignment exception
> case as it didn't call set_fs(KERNEL_DS).
>
> Nowadays, set_fs() is gone, previous patch fixed the alignment
> exception handler and __get_user/__put_user are not supposed to be
> used anymore to read kernel memory.
>
> Therefore the is_kernel_addr() check has become useless and can be
> removed.
While I agree that __get_user/__put_user should not be used on kernel
memory, I'm not sure that we have covered every case where they might be
used on kernel memory yet. I did a git grep for __get_user - there are
several callers in arch/powerpc and it looks like at least lib/sstep.c
might be using __get_user to read kernel memory while single-stepping.
I am not sure if might_sleep has got logic to cover the powerpc case -
it uses uaccess_kernel, but we don't supply a definition for that on
powerpc, so if we do end up calling __get_user on a kernel address, I
think we might now throw a warning. (Unless we are saved by
pagefault_disabled()?)
(But I haven't tested this yet, so it's possible I misunderstood
something.)
Do you expect any consequences if we've missed a case where
__(get|put)_user is called on a kernel address because it hasn't been
converted to use better helpers yet?
Kind regards,
Daniel
>
> Signed-off-by: Christophe Leroy <christophe.le...@csgroup.eu>
> ---
> arch/powerpc/include/asm/uaccess.h | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h
> b/arch/powerpc/include/asm/uaccess.h
> index eaa828a6a419..c4bbc64758a0 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -77,8 +77,7 @@ __pu_failed:
> \
> __typeof__(*(ptr)) __pu_val = (x); \
> __typeof__(size) __pu_size = (size); \
> \
> - if (!is_kernel_addr((unsigned long)__pu_addr)) \
> - might_fault(); \
> + might_fault(); \
> __chk_user_ptr(__pu_addr); \
> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> \
> @@ -238,12 +237,12 @@ do {
> \
> __typeof__(size) __gu_size = (size); \
> \
> __chk_user_ptr(__gu_addr); \
> - if (do_allow && !is_kernel_addr((unsigned long)__gu_addr)) \
> + if (do_allow) {
> \
> might_fault(); \
> - if (do_allow)
> \
> __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err);
> \
> - else
> \
> + } else {
> \
> __get_user_size_allowed(__gu_val, __gu_addr, __gu_size,
> __gu_err); \
> + }
> \
> (x) = (__typeof__(*(ptr)))__gu_val; \
> \
> __gu_err; \
> --
> 2.25.0
