AUDIT: copy_from_user is a deathtrap.

Are you sure they even exist ? As far as I can see near everybody relies on zeroing of target on exception instead.

At least for the SSE optimized copy_*_user always would be much better, because optimizing the miss count is painful from an unrolled loop and cannot be even done accurately (8 bytes accuracy is best with 8 byte loads/stored). With that in mind I think the byte count is broken by design because it cannot be correctly implemented unless you do byte copies.

I remember TCP was given as the prime user when this interface was introduced in 2.1, but TCP does not use the byte count currently and never has (in fact the primary memory copy interface of TCP - csum_copy_* - does not even support it)

-Andi

Oh, like read() or write() for regular files? Yup, they exist.

Linus

In message <[email protected]> you wr ite:

Huh? No, you ask for 2000 bytes into a buffer that can only take 1000 bytes without hitting an unmapped page, returning EFAULT or giving a SIGSEGV is perfectly acceptable.

As a coder, I'd *really* prefer that to hiding the bug!

There's only one case which really actually cares for a valid reason:
the hack in copying mount options.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Bzzt, wrong answer.

Partial reads/writes are perfectly possible and non-buggy for any system that uses variations of mmap/mprotect to implement user-level memory management, for example persistant data-bases, garbage collection etc.

Which means that if half of a buffer used for "read()" just happens to be marked non-writable for some GC purpose, the kernel HAS to have the ability return the right answer (which in this case is to say "I could only read 1000 bytes"). Because anything else just doesn't give the GC library (or whatever) any way to recover nicely.

Rusty, face it, you're wrong on this one. Just drop it.

Linus

In message <[email protected]> you wr ite:

Um, what about delivering a SIGSEGV? So, copy_to/from_user always returns 0, but a signal is delivered. Then the only places which need to be clever are the mount option copying, and the signal delivery code for SIGSEGV (which uses copy_to_user).

This has the benefit of not breaking existing kernel code, whichever interpretation of the return value is used.

That's certainly possible,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

That doesn't help. It's against some stupid SUS rule, I'm afraid.

(And THAT is a stupid rule, I 100% agree with. It means that some things return -EFAULT, and other things do SIGSEGV, and the only difference is whether something is a system call or is implemented as a library thing. UNIX should always just have segfaulted, but there you are..)

Linus

Hi!

I thought POSIX made it explicit that you may SIGSEGV or EFAULT at your option. If that SUS rule is stupid, we should just drop it.

Performance advantage from MMX-copy-to-user is probably well worth it.

Ouch, and your read example. Imagine you do read (fd, buf, 12000), and first page of buf is there, second is not and third is [could happen in your GC case]. What if copy-to-user decides to first write byte 11045 of buffer, then byte 17, then byte 4875 (and fault)? I think kernel *has* right to do that. What will it use as return value?

I think such GC library is seriously broken.
Pavel

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Stop this STUPID "it speeds things up" argument.

It's not TRUE.

We still have to do exactly the same things we do right now, because even if we SIGSEGV we still have to return the right number of bytes read/written.

SIGSEGV doesn't mean that the system call wouldn't complete. It removes _none_ of the kernel fixup handling, because the SIGSEGV won't be delivered until we return to user mode anyway. So please stop mixing these two issues up.

There are two completely orthogonal issues:

- Use SIGSEGV on system calls or not.

Using SIGSEGV makes the system call vs library routine issue more regular, but it does not change the fact that the system call has to return _something_.

- system call return value for partially successful read/write.

We already have the exact same issue wrt something like SIGINT, and nobody sane would ever suggest that we always return the "whole" thing if we're interrupted by an external signal.

Similarly, it's naive and stupid to suggest we return success if we get interrupted by a SIGSEGV/EFAULT.

On the first issue (SIGSEGV) I'm certainly open to trying that out, although I'm fairly certain there was _some_ reason we didn't do this a few years ago.

On the second issue, absolutely _nobody_ has shown any reason why we should break the existing code that does this correctly, and I've shown reasons why breaking it is STUPID.

Linus

Hi!

If you pass bad pointer to memcpy(), you don't expect any reasonable return value, right?

So if you pass bad pointer to read(), why would you expect "number of bytes read" return? Its true that kernel can't simply not return anything, but giving SIGSEGV and returning -EFAULT seems pretty reasonable to me. If they really want to, they might extract number of bytes read from address SIGSEGV occured at [but that's dirty hack, and people will hopefully realise that and not rely on it].
Pavel

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Because the standard says either you return the errorcode and no data is transferred or for a partial I/O you return how much was done. Its not neccessarily about accuracy either. If you do a 4k copy_from_user and error after for some reason returning -Esomething thats fine providing you didnt do anything that consumed data or shifted the file position etc

Hmm... I can't find anything like that in SuSv2: can you give a reference?

And we're already violating that for the write() case.

Rusty.

   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

The we should fix the bug. Its explicitly covered in SuSv3 because that incorporates the posix text. Its also discussed in things like 1003.1g drafts in great detail (sockets have very explicit ordering for error reporting)

Actually, if I pass a bad pointer to memcpy(), and I have a handler for the SIGSEGV that fixes up the thing, yes, by golly, I _do_ expect memcpy() to get the right value.

If it doesn't, then the system is BROKEN.

Face it Pavel, you're WRONG. You're so incredibly wrong that this is not worth even discussing. Linux does it right now, and I won't break that correct behaviour just because somebody has a incorrect and silly view of what is readable.

Face it, copy_to/from_user does the RIGHT THING, and stop whining about it.

Linus

Is it safe to stick a nose in here with some irrelevancies?

Pavel makes a reasonable point that copy_*_user may elect to copy the data in something other than strictly ascending user virtual addresses. In which case it's not possible to return a sane "how much was copied" number.

And copy_*_user is buggy at present: it doesn't correctly handle the case where the source and destination of the copy are overlapping in the same physical page. Example code below. One fix is to do the copy with descending addresses if src<dest or whatever. But then how to return the number of bytes??

Also, I see all these noises from x86 gurus about how copy_*_user() could be sped up heaps with fancy CPU-specific features. Could someone actually alight from butt and code that up?

akpm-1:/usr/src/ext3/tools> ./copy-user-test foo
aabcddfghhjkllnopprsttvwxx

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <sys/mman.h>

int main(int argc, char *argv[])

{
	int fd;
	char *filename;
	char *mapped_mem;
	char buf[26];
	int i;

	if (argc != 2) {
		fprintf(stderr, "Usage; %s filename\n", argv[0]);
		exit(1);
	}

	filename = argv[1];
	fd = open(filename, O_RDWR|O_TRUNC|O_CREAT, 0666);
	if (fd < 0) {
		fprintf(stderr, "%s: Cannot open `%s': %s\n",
			argv[0], filename, strerror(errno));
		exit(1);
	}

	for (i = 0; i < 26; i++)
		buf[i] = 'a' + i;

	mapped_mem = mmap(0, 26, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
	if (mapped_mem == 0) {
		perror("mmap");
		exit(1);
	}
	write(fd, buf, 26);
	lseek(fd, 1, SEEK_SET);
	write(fd, mapped_mem, 25);
	msync(mapped_mem, 26, MS_SYNC);
	munmap(mapped_mem, 26);
	close(fd);

	{
		char *p = malloc(strlen(filename) + 20);
		sprintf(p, "cat %s ; echo", filename);
		system(p);
	}
	exit(0);
}

I don't agree that that is true.

Do you have _any_ reasonable implementation taht would do that_

So we have memcpy() semantics for read()/write(), big deal.

The same way you aren't supposed to use memcpy() for overlapping areas, you're not supposed to read/write into such areas, for all the same reasons.

No. That wouldn't work anyway, because the addresses are totally different kinds.

The way we do now, which is the CORRECT way.

Stop this idiocy.

The current interface is quite well-defined, and has good semantics. Every single argument against it has been totally bogus, with no redeeming values.

Linus

Hi!

I believe I seen some strange memcpy for PPro (or something that obscure) that done out-of-order accesses to trigger prefetch logic. I can't find it any more, so I can't be sure...
Pavel

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Its festering quietly in the glibc source tree where all large and dubiously justifiable hacks seem to end up

Hi!

In such case, linus, here is your "reasonable" example. For PPro, it is faster to copy out-of-order, and if we wanted to use that for copy_to_user, you'd have your example.
Pavel

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Hi!

So... Is copy_to_user allowed to copy more than it actually reported? Because if so, it might return 0/-EFAULT as well ;-).

Pavel

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

The specification defines the API not the implementation. It only matters if the user can't tell. 0/-EFAULT would be wrong as that is EOF. 0/0 or -1/Efoo.

I think there is a misunderstanding here.

Nothing in the standards says that

write(pipe_fd, halfmappedbuffer, 2*PAGE_SIZE)

must return PAGE_SIZE on an error. What it seems to say is that it if an error is reported then no data got written down the actual pipe itself. Putting 4K into the pipe then reporting Esomething is not allowed. Copying 4K into a buffer faulting and erroring with Efoo then throwing away the buffer is allowed

write might be the easy case. But what about read? Is a failing read allowed to change the userspace memory?

Kasper Dupont -- der bruger for meget tid på usenet.
For sending spam use mailto:[email protected]

Dave actually tried this with the UDP receive code. The answer appears to be yes but not everything likes it when it occurs

In message <[email protected]> you write:

Hmmm... then noone is compliant AFAICT. Test program attached, which mprotects 100th page and tries to write 100 pages (interestingly, most OS's optimize writes to /dev/null, and always "succeed"):

OS Empty file 6 byte file Pipe
Return Size Return Size Valid Return Size

AIX EFAULT 99P EFAULT 99P+6 99P+6 EFAULT 97P

Linux 99P 100P 99P-6 100P 99P-2 99P 99P
(x86)

Solaris 98P 98P 99P-6 99P 99P EFAULT 98.75P

Key: Return = return value or error code if -1
Size = resulting file size
Valid = bytes written which were actually those requested P = * PAGE_SIZE

Summary: this is undefined behaviour, so I believe that we should do the simplest thing possible inside the kernel. I believe the simplest thing we can do is have the trap handler deliver SIGSEGV to the process, zero fill the region, and always return "success" to the caller. None of the callers need then care.

Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

/* Test for write from partially unmapped area.
Aligned output: ./test-write > /tmp/out
Unaligned output: echo hello > /tmp/out && ./test-write >> /tmp/out
(Note: check output with od -x /tmp/out)
Pipe: ./test-write | cat > /tmp/out
*/
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/mman.h>
#include <string.h>
#include <errno.h>
#include <limits.h>
#include <stdlib.h>
#include <stdio.h>

#define ALIGN(x,a) (((x)+(a)-1)&~((a)-1))

int main()

{
	int writeret;
	char *pages;
	long pagesize;

	pagesize = sysconf(_SC_PAGESIZE);
	fprintf(stderr, "Pagesize is %li\n", pagesize);

	pages = malloc(pagesize * 101);
	pages = (char *)ALIGN((unsigned long)pages, pagesize);
	memset(pages, 'A', pagesize*100);

	if (mprotect(pages + pagesize*99, pagesize, PROT_NONE) != 0) {
		perror("mprotect");
		exit(1);
	}

	writeret = write(STDOUT_FILENO, pages, pagesize*100);
	fprintf(stderr, "Write returned %i (%s)\n",
		writeret, writeret < 0 ? strerror(errno) : "no error");
	return 0;
}

How could you miss this one - the only one that gets it right? ;-)

Linux2.0 EFAULT 0 EFAULT 6 0 EFAULT 0

SCNR, ET.

Hey, you didn't read Alan's email.

The fact that glibc may use it is a strike _against_ your example.

Have you looked at glibc performance lately?

Linus

Hi!

:-).
Pavel

Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

If I understand Paulus correctly, PPC64 could share their optimized memcpy routine (ie. icache win), from which it is really hard to tell how far we got before we faulted.

You then do the fixup search on the link register (ie. to find the caller).

Rusty.

   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Well, if you can do it, I guess I can too...

Someone who knows x86 asm better than I can change everything in uaccess.h to always return 0, like so:

#define copy_from_user(to, from, n) \

({								\
	/* Returns non-zero only if hit fault handler */	\
	if (general_copy_user((to), (from), (n)))		\
		memset(to, 0, n);				\
	0;							\
})

So the fault handler sends a SIGSEGV, and makes generic_copy_user return 1 immediately (if you were ambitious, it could do the memset too).

If someone wants to write the asm for that, I'll fix the SEGV delivery code and mount option reading (two places where we must not deliver a signal), and we can see what it looks like.

Cheers,
Rusty.

   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

	    ^^^^^^^^^^^^^^^ minor reminder: you should check against MAP_FAILED here (-1 not 0)

very funny test really (it's like those games leading to an absurd), so you actually notice that copy_user reads and writes 4 byte at once till the last few bytes in the buffer :). I don't think anybody cares about those kind of semantics of physical page overlapping in a "word" range between pagecache destination and buffers source that is again the same phys page.

If we use mmx unrolled loops faster on the athlons the "word" granularity of copy_user would be 40bytes so it would be even more noticeable (by luck in your above testcase then it would actually write what you expect because the string is shorter and it wouldn't trigger a roll of the unrolled loop).

An easy fix to resurrect the "expected" semantics of write, is to read userspace and write pagecache in 1byte units, but that's slow, and I don't think it really worth. Let's declare this case undefined, it's not a security matter and it's not useful either I think.

other minor side note: msync actually cannot make differences to this case.

Andrea

[Cc: cleared]

^^^^^^^^^^^^^^^^^^^^

Because read() may *consume* its input, while memcpy() it's only a copy and leaves the source intact (if you care not to overlap src and dst). If you're read()ing from a serial line with 1-byte fifo you want to know how many chars you read successfully in the first place, since once read, they're gone. SIGSEGVing won't help in re-reading the chars.

.TM.

Linus is absolutely right. The correct semantics are to return the number of bytes read, if they are greater than zero, and on the next read return the error. This has been a corner case in read for a long time in various Unix versions, and Linus has it right. I went through this back at Sun and we explored all the different ways, and the bottom line is that you first ACK that you moved some data and then you NAK on the next read.

---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

In message <[email protected]> you write:

It's interesting to look at this backwards:

Imagine if copy_to_user returned void and delivered a SIGSEGV on fault, and always had.

Now, to fix this, we'd want to add new code paths to the 5,500 callers throughout the kernel.

I'm pretty sure everyone would balk. They'd say "sorry, you're going to have to wrap your syscalls somehow".

But as we all know, it is harder to remove a feature from Linux, than to get the camel book through the eye of a needle (or something).

Oh well,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

It's possible that I'm too tired to have grasped this, but if I have, you're all wet. In all cases, read needs to return the number of bytes successfully moved. If you ask for N and 1/2 of the way through N you are going to get a fault, and you return SEGFAULT, now how can I ever find out that N/2 bytes actually made it out to me? I want to know that. If you are arguing that return N/2 is wrong, you are incorrect.

---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

Hi!

You passed bad pointer to the kernel. You are broken. If you want to know that N/2 was delivered, fix your code not to pass bad pointers.
Pavel

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

Hi!

Imagine "I reallyu want to know how much memory memcpy() copied!". It is as unreasonable as that.
Pavel

Philips Velo 1: 1"x4"x8", 300gram, 60, 12MB, 40bogomips, linux, mutt,
details at http://atrey.karlin.mff.cuni.cz/~pavel/velo/index.html.

And that would be? I've never seen such an abomination. Sure, mapping pages on SEGV is use, but passing only partially mapped buffers to read/write on purpose???

Ciao, ET.

Some of the storage systems do this. They may try to use the host page size but on some platforms the host page size is variable so that isnt feasible.