From: Oleg Nesterov <oleg@redhat.com>
To: "Amanieu d'Antras" <amanieu@gmail.com>
Cc: linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
x86@kernel.org, Brian Gerst <brgerst@gmail.com>
Subject: Re: [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32
Date: Thu, 15 Oct 2015 20:41:58 +0200 [thread overview]
Message-ID: <20151015184158.GA30643@redhat.com> (raw)
In-Reply-To: <1444856371-26319-5-git-send-email-amanieu@gmail.com>
OOH ;) I'll try to look at this patch and the changes in the generic
code later. A couple of nits right now.
Please CC x86 maintainers, not only x86@kernel.org.
Please do not remove get/put_user_ex from this code. And this reminds
me that we can improve *user_try/*user_catch ...
On 10/14, Amanieu d'Antras wrote:
>
> -int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> +int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *from)
> {
> - int err = 0;
> + int err, si_code;
> bool ia32 = test_thread_flag(TIF_IA32);
>
> - if (!access_ok(VERIFY_WRITE, to, sizeof(compat_siginfo_t)))
> + if (!access_ok(VERIFY_WRITE, to, sizeof(siginfo_t)))
Why? This looks wrong.
> + if (from->si_code < 0) {
> + err |= __copy_to_user(to->_sifields._pad, from->_sifields._pad, SI_PAD_SIZE * sizeof(int))
> + ? -EFAULT : 0;
> + return err;
I think you should split this patch. And this change (don't interpet,
just copy) should go as a separate change.
> + switch (from->si_code & __SI_MASK) {
> + case __SI_KILL:
I agree, this looks better than ">> 16", but I'd suggest a separate
change too.
[...snip...]
the rest looks unreviewable because you didn't split it and because
you removed try/catch ;) The same for copy-from-user.
Please help us to understand these changes and make the more reviewable
patches if possible. Personally I think you have a point.
Oleg.
next prev parent reply other threads:[~2015-10-15 18:45 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-14 20:59 [PATCH 00/20] Fix handling of compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 01/20] compat: Add generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 02/20] compat: Add generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 03/20] x86: Update compat_siginfo_t to be closer to the generic version Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 04/20] x86: Rewrite copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 22:47 ` kbuild test robot
2015-10-15 18:41 ` Oleg Nesterov [this message]
2015-10-15 18:58 ` Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 05/20] mips: Clean up compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 06/20] mips: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 07/20] arm64: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 08/20] arm64: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 09/20] parisc: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 10/20] parsic: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 11/20] s390: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 12/20] s390: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 13/20] powerpc: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-16 23:00 ` kbuild test robot
2015-10-14 20:59 ` [PATCH 14/20] powerpc: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 15/20] tile: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 16/20] tile: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 17/20] sparc: Use generic compat_siginfo_t Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 18/20] sparc: Use generic copy_siginfo_{to,from}_user32 Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 19/20] signalfd: Fix handling of ssi_ptr and ssi_int in signalfd_copyinfo Amanieu d'Antras
2015-10-14 21:23 ` Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 19/20] signalfd: Fix some issues " Amanieu d'Antras
2015-10-14 20:59 ` [PATCH 20/20] signal: Remove unnecessary zero-initialization of siginfo_t Amanieu d'Antras
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=20151015184158.GA30643@redhat.com \
--to=oleg@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=amanieu@gmail.com \
--cc=brgerst@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=x86@kernel.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