From: Daniel Axtens <dja@axtens.net>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>
Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 07/15] powerpc/uaccess: Call might_fault() inconditionaly
Date: Fri, 26 Mar 2021 09:38:07 +1100 [thread overview]
Message-ID: <874kgykgfk.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <e0a980a4dc7a2551183dd5cb30f46eafdbee390c.1615398265.git.christophe.leroy@csgroup.eu>
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.leroy@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
next prev parent reply other threads:[~2021-03-25 22:38 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-10 17:46 [PATCH v2 00/15] powerpc: Cleanup of uaccess.h and adding asm goto for get_user() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap() Christophe Leroy
2021-03-10 21:47 ` Daniel Axtens
2021-03-10 17:46 ` [PATCH v2 02/15] powerpc/uaccess: Define ___get_user_instr() for ppc32 Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to user_access_begin Christophe Leroy
2021-03-10 22:31 ` Daniel Axtens
2021-03-11 5:45 ` Christophe Leroy
2021-03-12 13:25 ` [PATCH v3 " Christophe Leroy
2021-04-10 14:28 ` Michael Ellerman
2021-03-10 17:46 ` [PATCH v2 04/15] powerpc/uaccess: Remove __get/put_user_inatomic() Christophe Leroy
2021-03-10 22:37 ` Daniel Axtens
2021-03-10 17:46 ` [PATCH v2 05/15] powerpc/uaccess: Move get_user_instr helpers in asm/inst.h Christophe Leroy
2021-03-25 21:59 ` Daniel Axtens
2021-03-10 17:46 ` [PATCH v2 06/15] powerpc/align: Don't use __get_user_instr() on kernel addresses Christophe Leroy
2021-03-25 22:12 ` Daniel Axtens
2021-03-10 17:46 ` [PATCH v2 07/15] powerpc/uaccess: Call might_fault() inconditionaly Christophe Leroy
2021-03-25 22:38 ` Daniel Axtens [this message]
2021-03-25 22:44 ` Daniel Axtens
2021-03-10 17:46 ` [PATCH v2 08/15] powerpc/uaccess: Remove __unsafe_put_user_goto() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 09/15] powerpc/uaccess: Remove __chk_user_ptr() in __get/put_user Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 10/15] powerpc/uaccess: Remove calls to __get_user_bad() and __put_user_bad() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 11/15] powerpc/uaccess: Split out __get_user_nocheck() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 12/15] powerpc/uaccess: Rename __get/put_user_check/nocheck Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 13/15] powerpc/uaccess: Refactor get/put_user() and __get/put_user() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 14/15] powerpc/uaccess: Introduce __get_user_size_goto() Christophe Leroy
2021-03-10 17:46 ` [PATCH v2 15/15] powerpc/uaccess: Use asm goto for get_user when compiler supports it Christophe Leroy
2021-04-10 14:28 ` [PATCH v2 00/15] powerpc: Cleanup of uaccess.h and adding asm goto for get_user() Michael Ellerman
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=874kgykgfk.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens.net \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@csgroup.eu \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).