public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Borislav Petkov <bp@alien8.de>
Cc: linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Fenghua Yu <fenghua.yu@intel.com>,
	"H . Peter Anvin" <hpa@zytor.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Rik van Riel <riel@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Yu-cheng Yu <yu-cheng.yu@intel.com>
Subject: Re: [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs
Date: Mon, 30 Jan 2017 10:57:28 +0100	[thread overview]
Message-ID: <20170130095728.GA26142@gmail.com> (raw)
In-Reply-To: <20170127101656.r6djweg3snx4n7k2@pd.tnic>


* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Jan 26, 2017 at 11:22:49AM +0100, Ingo Molnar wrote:
> > The 'kbuf' parameter is unused in the _user() side of the API, remove it.
> > 
> > This simplifies the code and makes it easier to think about.
> 
> ...
> 
> > @@ -1010,10 +1010,7 @@ int copy_xstate_to_kernel(unsigned int pos, unsigned int count, void *kbuf, stru
> >  }
> >  
> >  static inline int
> > -__copy_xstate_to_user(unsigned int pos, unsigned int count,
> > -		      void *kbuf, void __user *ubuf,
> > -		      const void *data, const int start_pos,
> > -		      const int end_pos)
> > +__copy_xstate_to_user(unsigned int pos, unsigned int count, void __user *ubuf, const void *data, const int start_pos, const int end_pos)
> 
> That and similar lines are insanely long and could be broken.

Yeah, so that's one point of the series: the interface was insanely complex (the 
original sin is that of the regset interfaces), and one symptom of that complexity 
are these overly long prototypes - the above one has 7 arguments (!!). Another, 
far more serious symptom of the complexity were the bugs that Rik found.

The solution was not to break the prototype into multiple lines and thus paper 
over one symptom of complexity, but to _reduce_ complexity.

So at the end of the series the basic copy_xstate_to_user() prototype looks like 
this:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, unsigned int offset, unsigned int size, unsigned int size_total)

which is less complex and shorter as well. It could probably be shortened further:

 static inline int
 __copy_xstate_to_user(void __user *ubuf, const void *data, u32 offset, u32 size, u32 total)

because our regset (and user-copy) APIs are intentionally 32-bit - but this would 
depart from the existing signature style so I'm not sure we want to do it 
unilaterally.

Would anyone object to using u32 in these prototypes?

Thanks,

	Ingo

  reply	other threads:[~2017-01-30  9:58 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-26 10:22 [PATCH 00/14] x86/fpu: Clean up ptrace copying functions Ingo Molnar
2017-01-26 10:22 ` [PATCH 01/14] x86/fpu: Rename copyin_to_xsaves()/copyout_from_xsaves() to copy_user_to_xstate()/copy_xstate_to_user() Ingo Molnar
2017-01-26 10:22 ` [PATCH 02/14] x86/fpu: Split copy_xstate_to_user() into copy_xstate_to_kernel() & copy_xstate_to_user() Ingo Molnar
2017-01-26 10:22 ` [PATCH 03/14] x86/fpu: Remove 'ubuf' parameter from the copy_xstate_to_kernel() APIs Ingo Molnar
2017-01-26 10:22 ` [PATCH 04/14] x86/fpu: Remove 'kbuf' parameter from the copy_xstate_to_user() APIs Ingo Molnar
2017-01-27 10:16   ` Borislav Petkov
2017-01-30  9:57     ` Ingo Molnar [this message]
2017-01-30 15:45       ` Borislav Petkov
2017-01-30 17:23         ` Yu-cheng Yu
2017-01-26 10:22 ` [PATCH 05/14] x86/fpu: Clean up parameter order in the copy_xstate_to_*() APIs Ingo Molnar
2017-01-26 10:22 ` [PATCH 06/14] x86/fpu: Clean up the parameter definitions of copy_xstate_to_*() Ingo Molnar
2017-01-26 10:22 ` [PATCH 07/14] x86/fpu: Remove the 'start_pos' parameter from the __copy_xstate_to_*() functions Ingo Molnar
2017-01-26 10:22 ` [PATCH 08/14] x86/fpu: Clarify parameter names in the copy_xstate_to_*() methods Ingo Molnar
2017-01-26 10:22 ` [PATCH 09/14] x86/fpu: Change 'size_total' parameter to unsigned and standardize the size checks in copy_xstate_to_*() Ingo Molnar
2017-01-30 17:11   ` Yu-cheng Yu
2017-01-26 10:22 ` [PATCH 10/14] x86/fpu: Simplify __copy_xstate_to_kernel() return values Ingo Molnar
2017-01-26 10:22 ` [PATCH 11/14] x86/fpu: Split copy_user_to_xstate() into copy_kernel_to_xstate() & copy_user_to_xstate() Ingo Molnar
2017-01-27 10:54   ` Borislav Petkov
2017-01-26 10:22 ` [PATCH 12/14] x86/fpu: Remove 'ubuf' parameter from the copy_kernel_to_xstate() API Ingo Molnar
2017-01-26 10:22 ` [PATCH 13/14] x86/fpu: Remove 'kbuf' parameter from the copy_user_to_xstate() API Ingo Molnar
2017-01-26 10:22 ` [PATCH 14/14] x86/fpu: Flip the parameter order in copy_*_to_xstate() Ingo Molnar
2017-01-26 10:28 ` [PATCH 00/14] x86/fpu: Clean up ptrace copying functions Ingo Molnar

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=20170130095728.GA26142@gmail.com \
    --to=mingo@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=fenghua.yu@intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=riel@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=yu-cheng.yu@intel.com \
    /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