From: ebiederm@xmission.com (Eric W. Biederman) To: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Michael Ellerman <mpe@ellerman.id.au>, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user() Date: Wed, 08 Sep 2021 13:17:39 -0500 [thread overview] Message-ID: <877dfrrkxo.fsf@disp2133> (raw) In-Reply-To: <2715792c-eb10-eeb8-3d49-24486abe953b@csgroup.eu> (Christophe Leroy's message of "Fri, 3 Sep 2021 10:56:14 +0200") Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 02/09/2021 à 20:43, Eric W. Biederman a écrit : >> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> >>> In the same spirit as commit fb05121fd6a2 ("signal: Add >>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of >>> copy_siginfo_to_user() in order to use it within user access blocks. >>> >>> For that, also add an 'unsafe' version of clear_user(). >> >> Looking at your use cases you need the 32bit compat version of this >> as well. >> >> The 32bit compat version is too complicated to become a macro, so I >> don't think you can make this work correctly for the 32bit compat case. > > When looking into patch 5/5 that you nacked, I think you missed the fact that we > keep using copy_siginfo_to_user32() as it for the 32 bit compat case. I did. My mistake. However that mistake was so easy I think it mirrors the comments others have made that this looks like a maintenance hazard. Is improving the performance of 32bit kernels interesting? Is improving the performance of 32bit compat support interesting? If performance one or either of those cases is interesting it looks like we already have copy_siginfo_to_external32 the factor you would need to build unsafe_copy_siginfo_to_user32. So I am not going to say impossible but please make something maintainable. I unified all of the compat 32bit siginfo logic because it simply did not get enough love and attention when it was implemented per architecture. In general I think that concern applies to this case as well. We really need an implementation that shares as much burden as possible with other architectures. Eric >> Probably-Not-by: "Eric W. Biederman" <ebiederm@xmission.com> >> >> Eric >> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> --- >>> include/linux/signal.h | 15 +++++++++++++++ >>> include/linux/uaccess.h | 1 + >>> kernel/signal.c | 5 ----- >>> 3 files changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/signal.h b/include/linux/signal.h >>> index 3454c7ff0778..659bd43daf10 100644 >>> --- a/include/linux/signal.h >>> +++ b/include/linux/signal.h >>> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to, >>> int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from); >>> int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from); >>> +static __always_inline char __user *si_expansion(const siginfo_t __user >>> *info) >>> +{ >>> + return ((char __user *)info) + sizeof(struct kernel_siginfo); >>> +} >>> + >>> +#define unsafe_copy_siginfo_to_user(to, from, label) do { \ >>> + siginfo_t __user *__ucs_to = to; \ >>> + const kernel_siginfo_t *__ucs_from = from; \ >>> + char __user *__ucs_expansion = si_expansion(__ucs_to); \ >>> + \ >>> + unsafe_copy_to_user(__ucs_to, __ucs_from, \ >>> + sizeof(struct kernel_siginfo), label); \ >>> + unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label); \ >>> +} while (0) >>> + >>> enum siginfo_layout { >>> SIL_KILL, >>> SIL_TIMER, >>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h >>> index c05e903cef02..37073caac474 100644 >>> --- a/include/linux/uaccess.h >>> +++ b/include/linux/uaccess.h >>> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count); >>> #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e) >>> #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e) >>> #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e) >>> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e) >>> static inline unsigned long user_access_save(void) { return 0UL; } >>> static inline void user_access_restore(unsigned long flags) { } >>> #endif >>> diff --git a/kernel/signal.c b/kernel/signal.c >>> index a3229add4455..83b5971e4304 100644 >>> --- a/kernel/signal.c >>> +++ b/kernel/signal.c >>> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code) >>> return layout; >>> } >>> -static inline char __user *si_expansion(const siginfo_t __user *info) >>> -{ >>> - return ((char __user *)info) + sizeof(struct kernel_siginfo); >>> -} >>> - >>> int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from) >>> { >>> char __user *expansion = si_expansion(to);
WARNING: multiple messages have this Message-ID (diff)
From: ebiederm@xmission.com (Eric W. Biederman) To: Christophe Leroy <christophe.leroy@csgroup.eu> Cc: linuxppc-dev@lists.ozlabs.org, Paul Mackerras <paulus@samba.org>, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user() Date: Wed, 08 Sep 2021 13:17:39 -0500 [thread overview] Message-ID: <877dfrrkxo.fsf@disp2133> (raw) In-Reply-To: <2715792c-eb10-eeb8-3d49-24486abe953b@csgroup.eu> (Christophe Leroy's message of "Fri, 3 Sep 2021 10:56:14 +0200") Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 02/09/2021 à 20:43, Eric W. Biederman a écrit : >> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> >>> In the same spirit as commit fb05121fd6a2 ("signal: Add >>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of >>> copy_siginfo_to_user() in order to use it within user access blocks. >>> >>> For that, also add an 'unsafe' version of clear_user(). >> >> Looking at your use cases you need the 32bit compat version of this >> as well. >> >> The 32bit compat version is too complicated to become a macro, so I >> don't think you can make this work correctly for the 32bit compat case. > > When looking into patch 5/5 that you nacked, I think you missed the fact that we > keep using copy_siginfo_to_user32() as it for the 32 bit compat case. I did. My mistake. However that mistake was so easy I think it mirrors the comments others have made that this looks like a maintenance hazard. Is improving the performance of 32bit kernels interesting? Is improving the performance of 32bit compat support interesting? If performance one or either of those cases is interesting it looks like we already have copy_siginfo_to_external32 the factor you would need to build unsafe_copy_siginfo_to_user32. So I am not going to say impossible but please make something maintainable. I unified all of the compat 32bit siginfo logic because it simply did not get enough love and attention when it was implemented per architecture. In general I think that concern applies to this case as well. We really need an implementation that shares as much burden as possible with other architectures. Eric >> Probably-Not-by: "Eric W. Biederman" <ebiederm@xmission.com> >> >> Eric >> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> --- >>> include/linux/signal.h | 15 +++++++++++++++ >>> include/linux/uaccess.h | 1 + >>> kernel/signal.c | 5 ----- >>> 3 files changed, 16 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/signal.h b/include/linux/signal.h >>> index 3454c7ff0778..659bd43daf10 100644 >>> --- a/include/linux/signal.h >>> +++ b/include/linux/signal.h >>> @@ -35,6 +35,21 @@ static inline void copy_siginfo_to_external(siginfo_t *to, >>> int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from); >>> int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from); >>> +static __always_inline char __user *si_expansion(const siginfo_t __user >>> *info) >>> +{ >>> + return ((char __user *)info) + sizeof(struct kernel_siginfo); >>> +} >>> + >>> +#define unsafe_copy_siginfo_to_user(to, from, label) do { \ >>> + siginfo_t __user *__ucs_to = to; \ >>> + const kernel_siginfo_t *__ucs_from = from; \ >>> + char __user *__ucs_expansion = si_expansion(__ucs_to); \ >>> + \ >>> + unsafe_copy_to_user(__ucs_to, __ucs_from, \ >>> + sizeof(struct kernel_siginfo), label); \ >>> + unsafe_clear_user(__ucs_expansion, SI_EXPANSION_SIZE, label); \ >>> +} while (0) >>> + >>> enum siginfo_layout { >>> SIL_KILL, >>> SIL_TIMER, >>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h >>> index c05e903cef02..37073caac474 100644 >>> --- a/include/linux/uaccess.h >>> +++ b/include/linux/uaccess.h >>> @@ -398,6 +398,7 @@ long strnlen_user_nofault(const void __user *unsafe_addr, long count); >>> #define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e) >>> #define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(__copy_to_user(d,s,l),e) >>> #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e) >>> +#define unsafe_clear_user(d, l, e) unsafe_op_wrap(__clear_user(d, l), e) >>> static inline unsigned long user_access_save(void) { return 0UL; } >>> static inline void user_access_restore(unsigned long flags) { } >>> #endif >>> diff --git a/kernel/signal.c b/kernel/signal.c >>> index a3229add4455..83b5971e4304 100644 >>> --- a/kernel/signal.c >>> +++ b/kernel/signal.c >>> @@ -3261,11 +3261,6 @@ enum siginfo_layout siginfo_layout(unsigned sig, int si_code) >>> return layout; >>> } >>> -static inline char __user *si_expansion(const siginfo_t __user *info) >>> -{ >>> - return ((char __user *)info) + sizeof(struct kernel_siginfo); >>> -} >>> - >>> int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from) >>> { >>> char __user *expansion = si_expansion(to);
next prev parent reply other threads:[~2021-09-08 18:19 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-23 15:35 [PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block Christophe Leroy 2021-08-23 15:35 ` Christophe Leroy 2021-08-23 15:35 ` [PATCH v2 2/5] powerpc/signal: Include the new stack frame inside the " Christophe Leroy 2021-08-23 15:35 ` Christophe Leroy 2021-08-23 15:35 ` [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user() Christophe Leroy 2021-08-23 15:35 ` Christophe Leroy 2021-09-02 6:54 ` Christoph Hellwig 2021-09-02 6:54 ` Christoph Hellwig 2021-09-02 16:05 ` Linus Torvalds 2021-09-02 16:05 ` Linus Torvalds 2021-09-13 12:59 ` Christophe Leroy 2021-09-13 12:59 ` Christophe Leroy 2021-09-02 18:43 ` Eric W. Biederman 2021-09-02 18:43 ` Eric W. Biederman 2021-09-03 8:56 ` Christophe Leroy 2021-09-03 8:56 ` Christophe Leroy 2021-09-08 18:17 ` Eric W. Biederman [this message] 2021-09-08 18:17 ` Eric W. Biederman 2021-09-10 10:27 ` Christophe Leroy 2021-09-10 10:27 ` Christophe Leroy 2021-09-11 15:58 ` Eric W. Biederman 2021-09-11 15:58 ` Eric W. Biederman 2021-09-13 12:56 ` Christophe Leroy 2021-09-13 12:56 ` Christophe Leroy 2021-08-23 15:35 ` [PATCH v2 4/5] powerpc/uaccess: Add unsafe_clear_user() Christophe Leroy 2021-08-23 15:35 ` Christophe Leroy 2021-08-23 15:35 ` [PATCH v2 5/5] powerpc/signal: Use unsafe_copy_siginfo_to_user() Christophe Leroy 2021-08-23 15:35 ` Christophe Leroy 2021-09-02 18:38 ` Eric W. Biederman 2021-09-02 18:38 ` Eric W. Biederman 2021-09-03 8:53 ` Christophe Leroy 2021-09-03 8:53 ` Christophe Leroy 2021-09-02 6:49 ` [PATCH v2 1/5] powerpc/signal64: Access function descriptor with user access block Christoph Hellwig 2021-09-02 6:49 ` Christoph Hellwig
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=877dfrrkxo.fsf@disp2133 \ --to=ebiederm@xmission.com \ --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: 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.