LKML Archive on
help / color / mirror / Atom feed
From: Linus Torvalds <>
To: "Michael S. Tsirkin" <>
Cc: Al Viro <>,
	Linux Kernel Mailing List <>,
	stable <>,,
	linux-mm <>,
	"Kirill A. Shutemov" <>,
	Andrew Morton <>,
	Huang Ying <>,
	Jonathan Corbet <>,
	Peter Zijlstra <>,
	Thomas Gleixner <>,
	Thorsten Leemhuis <>
Subject: Re: [PATCH] gup: return -EFAULT on access_ok failure
Date: Thu, 5 Apr 2018 11:43:27 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Apr 5, 2018 at 11:28 AM, Michael S. Tsirkin <> wrote:
> to repeat what you are saying IIUC __get_user_pages_fast returns 0 if it can't
> pin any pages and that is by design.  Returning 0 on error isn't usual I think
> so I guess this behaviour should we well documented.

Arguably it happens elsewhere too, and not just in the kernel.
"read()" at past the end of a file is not an error, you'll just get 0
for EOF.

So it's not really "returning 0 on error".

It really is simply returning the number of pages it got. End of
story. That number of pages can be smaller than the requested number
of pages, and _that_ is due to some error, but note how it can return
"5" on error too - you asked for 10 pages, but the error happened in
the middle!

So the right way to check for error is to bverify that you get the
number of pages that you asked for. If you don't, something bad

Of course, many users don't actually care about "I didn't get
everything". They only care about "did I get _something_". Then that 0
ends up being the error case, but note how it depends on the caller.

> What about get_user_pages_fast though?

We do seem to special-case the first page there. I'm not sure it's a
good idea. But like the __get_user_pages_fast(), we seem to have users
that know about the particular semantics and depend on it.

It's all ugly, I agree.

End result: we can't just change semantics of either of them.

At least not without going through every single user and checking that
they are ok with it.

Which I guess I could be ok with. Maybe changing the semantics of
__get_user_pages_fast() is acceptable, if you just change it
*everywhere* (which includes not just he users, but also the couple of
architecture-specific versions of that same function that we have.


  reply	other threads:[~2018-04-05 18:43 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
2018-04-05  1:53 ` Michael S. Tsirkin
2018-04-05  2:40   ` Linus Torvalds
2018-04-05 14:17     ` Michael S. Tsirkin
2018-04-05 15:40       ` Linus Torvalds
2018-04-05 18:28         ` Michael S. Tsirkin
2018-04-05 18:43           ` Linus Torvalds [this message]
2018-04-05 19:34             ` Michael S. Tsirkin
2018-04-05 19:39               ` Chris Wilson
2018-04-05 21:08             ` Michael S. Tsirkin
2018-04-06 11:35           ` Alan Cox

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH] gup: return -EFAULT on access_ok failure' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).