[RFC] raw_copy_from_user() semantics

[RFC] raw_copy_from_user() semantics

[Al Viro]

	Back in 2017 I'd made a bogus promise regarding raw_copy_from_user().
 Namely, that in case of short copy it will copy at least something unless
nothing could've been read at all.

	Such property could've been used by code that would want to squeeze
every byte, by doing copy_from_user() in a loop, advancing the source and
destination by the amount copied until we stop getting any progress.

	There are two problems with that.  First of all, the promise was
bogus - there are architectures where it is simply not true.  E.g. ppc
(or alpha, or sparc, or...)  can have copy_from_user() called with source
one word prior to an unmapped page, fetch that word, fail to fetch the next
one and bugger off without doing any stores.

	So any byte-squeezing loop of that sort would break on a bunch
of architectures.  Another problem is that we simply don't have a lot
of such loops.	Looking through the mainline kernel has found only two
such beasts - one in vcs_write() and another in iomap_dio_inline_actor().
Both are in write(2) and write(2) has never made any warranties regarding
the situation when a part of buffer we want to write is unreadable.
In particular, it does *not* squeeze every last byte out - not for
regular files on local filesystems, not for NFS, not for pipes, not for
sockets, etc.

	E.g. in case of generic_perform_write() we treat "nothing has
been copied" as "try to fault in, fail if that's not possible, retry the
attempt to copy otherwise"; we have to, since we are doing the copying
with pagefaults disabled, so a failure to copy anything may be simply
due to a page evicted by memory pressure.  And if we have an evicted page
followed by genuinely unreadable one, we can very well have fault-in fail
(and write stop) while we still have a couple of kilobytes worth of data
yet to be copied from the evicted page.

	We could try to make the promise true; that would mean messing
with a lot of unpleasant asm code (in exception handlers, at that) and
I don't believe it's worth the bother.  We could add a "squeeze every
last byte" helper, but I don't see any valid users for it right now.

	BTW, "do copy_from_user() in a loop" is not a good way to do such
helper anyway - if we stop copying on a fault and lose some data already
fetched, that data is not going to be more than a few registers worth,
so it's better to have this helper do a byte-by-byte get_user() loop
for dealing with that case.

	IMO we should simply remove the bogus promise, document the real
situation and if somebody needs the "squeeze every last byte" helper,
let them say so.  Audit of the current mainline shows only two places
that might be trying something like that and AFAICS both are accidental.

	Does anybody object against the following?

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 7bcadca22100..083fda6aa384 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -30,10 +30,11 @@
  * starting at from.  All data past to + size - N must be left unmodified.
  *
  * If copying succeeds, the return value must be 0.  If some data cannot be
- * fetched, it is permitted to copy less than had been fetched; the only
- * hard requirement is that not storing anything at all (i.e. returning size)
- * should happen only when nothing could be copied.  In other words, you don't
- * have to squeeze as much as possible - it is allowed, but not necessary.
+ * fetched, it is permitted to copy less than had been fetched.  In other
+ * words, you don't have to squeeze as much as possible - it is allowed, but
+ * not necessary.  In particular, it is possible to have some bytes fetched
+ * and nothing copied, so do _not_ assume that doing copies until we get to
+ * "nothing copied" will copy every byte that can be fetched.
  *
  * For raw_copy_from_user() to always points to kernel memory and no faults
  * on store should happen.  Interpretation of from is affected by set_fs().

Re: [RFC] raw_copy_from_user() semantics

[Linus Torvalds]

On Sat, Jul 18, 2020 at 8:17 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         So any byte-squeezing loop of that sort would break on a bunch
> of architectures.

I think we should try to get rid of the exact semantics.

If "copy_from/to_user()" takes a fault because it does a
larger-than-byte access (and with unrolling, it could be a _lot_
larger than one byte: x86 dcurrently has that "generic" case that
isn't used very much, but it unrolls 8-byte accesses 8 times, so it
does a 64-byte block that we could just say "if any fo those didn't
work, then you're done), then the copy failed. The exact number of
bytes we _could_ have copied is not important.

So we could simplify the x86 end condition too and remove all the
"handle_tail" complexity.

                  Linus

(*) Yes, it aligns things to 64-byte boundaries too, but only for the
write side, not the read side.

Re: [RFC] raw_copy_from_user() semantics

[Linus Torvalds]

On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think we should try to get rid of the exact semantics.

Side note: I think one of the historical reasons for the exact
semantics was that we used to do things like the mount option copying
with a "copy_from_user()" iirc.

And that could take a fault at the end of the stack etc, because
"copy_mount_options()" is nasty and doesn't get a size, and just
copies "up to 4kB" of data.

It's a mistake in the interface, but it is what it is. But we've
always handled the inexact count there anyway by originally doing byte
accesses, and at some point you optimized it to just look at where
page boundaries might be..

I think that was the only truly _valid_ case of "we actually copy data
from user space, and we might need to handle a partial case", and
exactly because of that, it had already long avoided the whole "assume
copy_from_user gives us byte-accurate data before the fault".

              Linus

Re: [RFC] raw_copy_from_user() semantics

[Catalin Marinas]

On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > I think we should try to get rid of the exact semantics.
> 
> Side note: I think one of the historical reasons for the exact
> semantics was that we used to do things like the mount option copying
> with a "copy_from_user()" iirc.
> 
> And that could take a fault at the end of the stack etc, because
> "copy_mount_options()" is nasty and doesn't get a size, and just
> copies "up to 4kB" of data.
> 
> It's a mistake in the interface, but it is what it is. But we've
> always handled the inexact count there anyway by originally doing byte
> accesses, and at some point you optimized it to just look at where
> page boundaries might be..

And we may have to change this again since, with arm64 MTE, the page
boundary check is insufficient:

https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/

While currently the fault path is unlikely to trigger, with MTE in user
space it's a lot more likely since the buffer (e.g. a string) is
normally less than 4K and the adjacent addresses would have a different
colour.

I looked (though briefly) into passing the copy_from_user() problem to
filesystems that would presumably know better how much to copy. In most
cases the options are string, so something like strncpy_from_user()
would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
has a better way to figure out how much to copy.

> I think that was the only truly _valid_ case of "we actually copy data
> from user space, and we might need to handle a partial case", and
> exactly because of that, it had already long avoided the whole "assume
> copy_from_user gives us byte-accurate data before the fault".

With MTE, we didn't find any other instance of copy_from_user() where
the byte accuracy matters. The close relative, strncpy_from_user(),
already handles exact copying via a fall back to byte at a time.

-- 
Catalin

RE: [RFC] raw_copy_from_user() semantics

[David Laight]

From: Catalin Marinas
> Sent: 22 July 2020 12:37
> 
> On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> > On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > I think we should try to get rid of the exact semantics.
> >
> > Side note: I think one of the historical reasons for the exact
> > semantics was that we used to do things like the mount option copying
> > with a "copy_from_user()" iirc.
> >
> > And that could take a fault at the end of the stack etc, because
> > "copy_mount_options()" is nasty and doesn't get a size, and just
> > copies "up to 4kB" of data.
> >
> > It's a mistake in the interface, but it is what it is. But we've
> > always handled the inexact count there anyway by originally doing byte
> > accesses, and at some point you optimized it to just look at where
> > page boundaries might be..
> 
> And we may have to change this again since, with arm64 MTE, the page
> boundary check is insufficient:
> 
> https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/
> 
> While currently the fault path is unlikely to trigger, with MTE in user
> space it's a lot more likely since the buffer (e.g. a string) is
> normally less than 4K and the adjacent addresses would have a different
> colour.
> 
> I looked (though briefly) into passing the copy_from_user() problem to
> filesystems that would presumably know better how much to copy. In most
> cases the options are string, so something like strncpy_from_user()
> would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
> has a better way to figure out how much to copy.

What about changing the mount code to loop calling get_user()
to read aligned words until failure?
Mount is fairly uncommon and the extra cost is probably small compared
to the rest of doing a mount.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC] raw_copy_from_user() semantics

[Catalin Marinas]

On Wed, Jul 22, 2020 at 01:14:21PM +0000, David Laight wrote:
> From: Catalin Marinas
> > Sent: 22 July 2020 12:37
> > On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> > > On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > > I think we should try to get rid of the exact semantics.
> > >
> > > Side note: I think one of the historical reasons for the exact
> > > semantics was that we used to do things like the mount option copying
> > > with a "copy_from_user()" iirc.
> > >
> > > And that could take a fault at the end of the stack etc, because
> > > "copy_mount_options()" is nasty and doesn't get a size, and just
> > > copies "up to 4kB" of data.
> > >
> > > It's a mistake in the interface, but it is what it is. But we've
> > > always handled the inexact count there anyway by originally doing byte
> > > accesses, and at some point you optimized it to just look at where
> > > page boundaries might be..
> > 
> > And we may have to change this again since, with arm64 MTE, the page
> > boundary check is insufficient:
> > 
> > https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/
> > 
> > While currently the fault path is unlikely to trigger, with MTE in user
> > space it's a lot more likely since the buffer (e.g. a string) is
> > normally less than 4K and the adjacent addresses would have a different
> > colour.
> > 
> > I looked (though briefly) into passing the copy_from_user() problem to
> > filesystems that would presumably know better how much to copy. In most
> > cases the options are string, so something like strncpy_from_user()
> > would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
> > has a better way to figure out how much to copy.
> 
> What about changing the mount code to loop calling get_user()
> to read aligned words until failure?
> Mount is fairly uncommon and the extra cost is probably small compared
> to the rest of doing a mount.

Before commit 12efec560274 ("saner copy_mount_options()"), it was using
single-byte get_user(). That could have been optimised for aligned words
reading but I don't really think it's worth the hassle. Since the source
and destination don't have the same alignment and some architecture
don't support unaligned accesses (for storing to the kernel buffer), it
would just make this function unnecessarily complicated.

-- 
Catalin

RE: [RFC] raw_copy_from_user() semantics

[David Laight]

From: Catalin Marinas
> Sent: 22 July 2020 17:54
> 
> On Wed, Jul 22, 2020 at 01:14:21PM +0000, David Laight wrote:
> > From: Catalin Marinas
> > > Sent: 22 July 2020 12:37
> > > On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> > > > On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> > > > <torvalds@linux-foundation.org> wrote:
> > > > > I think we should try to get rid of the exact semantics.
> > > >
> > > > Side note: I think one of the historical reasons for the exact
> > > > semantics was that we used to do things like the mount option copying
> > > > with a "copy_from_user()" iirc.
> > > >
> > > > And that could take a fault at the end of the stack etc, because
> > > > "copy_mount_options()" is nasty and doesn't get a size, and just
> > > > copies "up to 4kB" of data.
> > > >
> > > > It's a mistake in the interface, but it is what it is. But we've
> > > > always handled the inexact count there anyway by originally doing byte
> > > > accesses, and at some point you optimized it to just look at where
> > > > page boundaries might be..
> > >
> > > And we may have to change this again since, with arm64 MTE, the page
> > > boundary check is insufficient:
> > >
> > > https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/
> > >
> > > While currently the fault path is unlikely to trigger, with MTE in user
> > > space it's a lot more likely since the buffer (e.g. a string) is
> > > normally less than 4K and the adjacent addresses would have a different
> > > colour.
> > >
> > > I looked (though briefly) into passing the copy_from_user() problem to
> > > filesystems that would presumably know better how much to copy. In most
> > > cases the options are string, so something like strncpy_from_user()
> > > would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
> > > has a better way to figure out how much to copy.
> >
> > What about changing the mount code to loop calling get_user()
> > to read aligned words until failure?
> > Mount is fairly uncommon and the extra cost is probably small compared
> > to the rest of doing a mount.
> 
> Before commit 12efec560274 ("saner copy_mount_options()"), it was using
> single-byte get_user(). That could have been optimised for aligned words
> reading but I don't really think it's worth the hassle. Since the source
> and destination don't have the same alignment and some architecture
> don't support unaligned accesses (for storing to the kernel buffer), it
> would just make this function unnecessarily complicated.

It could do aligned words if the user buffer is aligned (it will be
most of the time) and bytes otherwise.

Or just fallback to a byte loop if the full 4k read fails.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC] raw_copy_from_user() semantics

[Catalin Marinas]

On Thu, Jul 23, 2020 at 08:37:27AM +0000, David Laight wrote:
> From: Catalin Marinas
> > Sent: 22 July 2020 17:54
> > On Wed, Jul 22, 2020 at 01:14:21PM +0000, David Laight wrote:
> > > From: Catalin Marinas
> > > > Sent: 22 July 2020 12:37
> > > > On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> > > > > On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> > > > > <torvalds@linux-foundation.org> wrote:
> > > > > > I think we should try to get rid of the exact semantics.
> > > > >
> > > > > Side note: I think one of the historical reasons for the exact
> > > > > semantics was that we used to do things like the mount option copying
> > > > > with a "copy_from_user()" iirc.
> > > > >
> > > > > And that could take a fault at the end of the stack etc, because
> > > > > "copy_mount_options()" is nasty and doesn't get a size, and just
> > > > > copies "up to 4kB" of data.
> > > > >
> > > > > It's a mistake in the interface, but it is what it is. But we've
> > > > > always handled the inexact count there anyway by originally doing byte
> > > > > accesses, and at some point you optimized it to just look at where
> > > > > page boundaries might be..
> > > >
> > > > And we may have to change this again since, with arm64 MTE, the page
> > > > boundary check is insufficient:
> > > >
> > > > https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/
> > > >
> > > > While currently the fault path is unlikely to trigger, with MTE in user
> > > > space it's a lot more likely since the buffer (e.g. a string) is
> > > > normally less than 4K and the adjacent addresses would have a different
> > > > colour.
> > > >
> > > > I looked (though briefly) into passing the copy_from_user() problem to
> > > > filesystems that would presumably know better how much to copy. In most
> > > > cases the options are string, so something like strncpy_from_user()
> > > > would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
> > > > has a better way to figure out how much to copy.
> > >
> > > What about changing the mount code to loop calling get_user()
> > > to read aligned words until failure?
> > > Mount is fairly uncommon and the extra cost is probably small compared
> > > to the rest of doing a mount.
> > 
> > Before commit 12efec560274 ("saner copy_mount_options()"), it was using
> > single-byte get_user(). That could have been optimised for aligned words
> > reading but I don't really think it's worth the hassle. Since the source
> > and destination don't have the same alignment and some architecture
> > don't support unaligned accesses (for storing to the kernel buffer), it
> > would just make this function unnecessarily complicated.
> 
> It could do aligned words if the user buffer is aligned (it will be
> most of the time) and bytes otherwise.
> 
> Or just fallback to a byte loop if the full 4k read fails.

That's what I'm proposing here (needed for arm64 MTE):

https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/

-- 
Catalin

RE: [RFC] raw_copy_from_user() semantics

[David Laight]

From: Catalin Marinas 
> Sent: 23 July 2020 11:19
> On Thu, Jul 23, 2020 at 08:37:27AM +0000, David Laight wrote:
> > From: Catalin Marinas
> > > Sent: 22 July 2020 17:54
> > > On Wed, Jul 22, 2020 at 01:14:21PM +0000, David Laight wrote:
> > > > From: Catalin Marinas
> > > > > Sent: 22 July 2020 12:37
> > > > > On Sun, Jul 19, 2020 at 12:34:11PM -0700, Linus Torvalds wrote:
> > > > > > On Sun, Jul 19, 2020 at 12:28 PM Linus Torvalds
> > > > > > <torvalds@linux-foundation.org> wrote:
> > > > > > > I think we should try to get rid of the exact semantics.
> > > > > >
> > > > > > Side note: I think one of the historical reasons for the exact
> > > > > > semantics was that we used to do things like the mount option copying
> > > > > > with a "copy_from_user()" iirc.
> > > > > >
> > > > > > And that could take a fault at the end of the stack etc, because
> > > > > > "copy_mount_options()" is nasty and doesn't get a size, and just
> > > > > > copies "up to 4kB" of data.
> > > > > >
> > > > > > It's a mistake in the interface, but it is what it is. But we've
> > > > > > always handled the inexact count there anyway by originally doing byte
> > > > > > accesses, and at some point you optimized it to just look at where
> > > > > > page boundaries might be..
> > > > >
> > > > > And we may have to change this again since, with arm64 MTE, the page
> > > > > boundary check is insufficient:
> > > > >
> > > > > https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/
> > > > >
> > > > > While currently the fault path is unlikely to trigger, with MTE in user
> > > > > space it's a lot more likely since the buffer (e.g. a string) is
> > > > > normally less than 4K and the adjacent addresses would have a different
> > > > > colour.
> > > > >
> > > > > I looked (though briefly) into passing the copy_from_user() problem to
> > > > > filesystems that would presumably know better how much to copy. In most
> > > > > cases the options are string, so something like strncpy_from_user()
> > > > > would work. For mount options as binary blobs (IIUC btrfs) maybe the fs
> > > > > has a better way to figure out how much to copy.
> > > >
> > > > What about changing the mount code to loop calling get_user()
> > > > to read aligned words until failure?
> > > > Mount is fairly uncommon and the extra cost is probably small compared
> > > > to the rest of doing a mount.
> > >
> > > Before commit 12efec560274 ("saner copy_mount_options()"), it was using
> > > single-byte get_user(). That could have been optimised for aligned words
> > > reading but I don't really think it's worth the hassle. Since the source
> > > and destination don't have the same alignment and some architecture
> > > don't support unaligned accesses (for storing to the kernel buffer), it
> > > would just make this function unnecessarily complicated.
> >
> > It could do aligned words if the user buffer is aligned (it will be
> > most of the time) and bytes otherwise.
> >
> > Or just fallback to a byte loop if the full 4k read fails.
> 
> That's what I'm proposing here (needed for arm64 MTE):
> 
> https://lore.kernel.org/linux-fsdevel/20200715170844.30064-25-catalin.marinas@arm.com/

Seems not unreasonable...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)