pwritev02 hang on s390x with 4.8.0-rc7

Hi,

I'm hitting a regression with LTP's pwritev02 [1] on s390x with 4.8.0-rc7. Specifically the EFAULT case, which is passing an iovec with invalid base address:
#define CHUNK 64
static struct iovec wr_iovec3[] = {
{(char *)-1, CHUNK},
};
hangs with 100% cpu usage and not very helpful stack trace:

  # cat /proc/28544/stack
  [<0000000000001000>] 0x1000
  [<ffffffffffffffff>] 0xffffffffffffffff

The problem starts with d4690f1e1cda "fix iov_iter_fault_in_readable()".

Before this commit fault_in_pages_readable() called __get_user() on start address which presumably failed with -EFAULT immediately.

With this commit applied fault_in_multipages_readable() appears to return 0 for the case when "start" is invalid but "end" overflows. Then according to my traces we keep spinning inside while loop in iomap_write_actor().

Patch below makes the issue go away for me:

index 66a1260b33de..e443dbd2b5df 100644
@@ -600,11 +600,11 @@ static inline int fault_in_multipages_readable(const char __user *uaddr,
                                               int size)
 {
        volatile char c;
-       int ret = 0;
+       int ret = -EFAULT;
        const char __user *end = uaddr + size - 1;
 
        if (unlikely(size == 0))
-               return ret;
+               return 0;
 
        while (uaddr <= end) {
                ret = __get_user(c, uaddr);

Regards,
Jan

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/pwritev/pwritev02.c

Cute. Let me see if I understand what's going on there: we have a wraparound that would've been caught by most of access_ok(), but not on an architectures where access_ok() is a no-op; in that case the loop is skipped and we just check the last address, which passes and we get a false positive. Bug is real and it's definitely -stable fodder.

I'm not sure that the fix you propose is right, though. Note that ERR_PTR() is not a valid address on any architecture, so any wraparound automatically means -EFAULT and we can simply check unlikely(uaddr > end) and bugger off if it holds. while() turns into do-while(), of course, and the same is needed for the read side.

Could you check if the following works for you?

index 66a1260..7e3d537 100644
@@ -571,56 +571,56 @@ static inline int fault_in_pages_readable(const char __user *uaddr, int size)
  */
 static inline int fault_in_multipages_writeable(char __user *uaddr, int size)
 {
-	int ret = 0;
 	char __user *end = uaddr + size - 1;
 
 	if (unlikely(size == 0))
-		return ret;
+		return 0;
 
+	if (unlikely(uaddr > end))
+		return -EFAULT;
 	/*
 	 * Writing zeroes into userspace here is OK, because we know that if
 	 * the zero gets there, we'll be overwriting it.
 	 */
-	while (uaddr <= end) {
-		ret = __put_user(0, uaddr);
-		if (ret != 0)
-			return ret;
+	do {
+		if (unlikely(__put_user(0, uaddr) != 0))
+			return -EFAULT;
 		uaddr += PAGE_SIZE;
-	}
+	} while (uaddr <= end);
 
 	/* Check whether the range spilled into the next page. */
 	if (((unsigned long)uaddr & PAGE_MASK) ==
 			((unsigned long)end & PAGE_MASK))
-		ret = __put_user(0, end);
+		return __put_user(0, end);
 
-	return ret;
+	return 0;
 }
 
 static inline int fault_in_multipages_readable(const char __user *uaddr,
 					       int size)
 {
 	volatile char c;
-	int ret = 0;
 	const char __user *end = uaddr + size - 1;
 
 	if (unlikely(size == 0))
-		return ret;
+		return 0;
 
-	while (uaddr <= end) {
-		ret = __get_user(c, uaddr);
-		if (ret != 0)
-			return ret;
+	if (unlikely(uaddr > end))
+		return -EFAULT;
+
+	do {
+		if (unlikely(__get_user(c, uaddr) != 0))
+			return -EFAULT;
 		uaddr += PAGE_SIZE;
-	}
+	} while (uaddr <= end);
 
 	/* Check whether the range spilled into the next page. */
 	if (((unsigned long)uaddr & PAGE_MASK) ==
 			((unsigned long)end & PAGE_MASK)) {
-		ret = __get_user(c, end);
-		(void)c;
+		return __get_user(c, end);
 	}
 
-	return ret;
+	return 0;
 }
 
 int add_to_page_cache_locked(struct page *page, struct address_space *mapping,

----- Original Message -----

This fixes pwritev02 hang.

I ran all syscalls tests from LTP, and I see a change in behaviour of couple other tests (writev01, writev03 and writev04 [1]) in 4.8.0-rc7.

These call writev() with partially invalid iovecs, and now fail with EFAULT, while with previous -rc6 kernel they returned number of bytes written before they encountered invalid iovec record.
This should be reproducible also on x86.

Regards,
Jan

[1] https://github.com/linux-test-project/ltp/tree/master/testcases/kernel/syscalls/writev

Known, discussed and considered legitimate. It's not so much EFAULT vs short write, it's how far do we shorten the write. Change consists of removing an accidental (and undocumented) property of iovec boundaries wrt write shortening. Usually an invalid address anywhere in the data we are asked to write leads to write shortened to the last pagecache boundary (i.e file position multiple of page size) entirely covered by valid data. It is filesystem-dependent and already deep in nasal demon territory. writev, pretty much by accident, never shortened past an iovec boundary. That's what got changed - now the rules are same as they are for all writes.

Having an LTP test (as opposed to actual real-world code) deliberately stepping into that and checking how far does the shortening go means just one thing: update the test.

fyi

----- Forwarded Message -----
From: "Al Viro" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 20 September, 2016 7:30:33 PM
Subject: Re: [bug] pwritev02 hang on s390x with 4.8.0-rc7

Known, discussed and considered legitimate. It's not so much EFAULT vs short write, it's how far do we shorten the write. Change consists of removing an accidental (and undocumented) property of iovec boundaries wrt write shortening. Usually an invalid address anywhere in the data we are asked to write leads to write shortened to the last pagecache boundary (i.e file position multiple of page size) entirely covered by valid data. It is filesystem-dependent and already deep in nasal demon territory. writev, pretty much by accident, never shortened past an iovec boundary. That's what got changed - now the rules are same as they are for all writes.

Having an LTP test (as opposed to actual real-world code) deliberately stepping into that and checking how far does the shortening go means just one thing: update the test.

Switching iov_iter fault-in to multipages variants has exposed an old bug in underlying fault_in_multipages_...(); they break if the range passed to them wraps around. Normally access_ok() done by callers will prevent such (and it's a guaranteed EFAULT - ERR_PTR() values fall into such a range and they should not point to any valid objects). However, on architectures where userland and kernel live in different MMU contexts (e.g. s390) access_ok() is a no-op and on those a range with a wraparound can reach fault_in_multipages_...().

Since any wraparound means EFAULT there, the fix is trivial - turn those
while (uaddr <= end)
...
into

    if (unlikely(uaddr > end))
	    return -EFAULT;
    do
	    ...
    while (uaddr <= end);

index 66a1260..7e3d537 100644
@@ -571,56 +571,56 @@ static inline int fault_in_pages_readable(const char __user *uaddr, int size)
  */
 static inline int fault_in_multipages_writeable(char __user *uaddr, int size)
 {
-	int ret = 0;
 	char __user *end = uaddr + size - 1;
 
 	if (unlikely(size == 0))
-		return ret;
+		return 0;
 
+	if (unlikely(uaddr > end))
+		return -EFAULT;
 	/*
 	 * Writing zeroes into userspace here is OK, because we know that if
 	 * the zero gets there, we'll be overwriting it.
 	 */
-	while (uaddr <= end) {
-		ret = __put_user(0, uaddr);
-		if (ret != 0)
-			return ret;
+	do {
+		if (unlikely(__put_user(0, uaddr) != 0))
+			return -EFAULT;
 		uaddr += PAGE_SIZE;
-	}
+	} while (uaddr <= end);
 
 	/* Check whether the range spilled into the next page. */
 	if (((unsigned long)uaddr & PAGE_MASK) ==
 			((unsigned long)end & PAGE_MASK))
-		ret = __put_user(0, end);
+		return __put_user(0, end);
 
-	return ret;
+	return 0;
 }
 
 static inline int fault_in_multipages_readable(const char __user *uaddr,
 					       int size)
 {
 	volatile char c;
-	int ret = 0;
 	const char __user *end = uaddr + size - 1;
 
 	if (unlikely(size == 0))
-		return ret;
+		return 0;
 
-	while (uaddr <= end) {
-		ret = __get_user(c, uaddr);
-		if (ret != 0)
-			return ret;
+	if (unlikely(uaddr > end))
+		return -EFAULT;
+
+	do {
+		if (unlikely(__get_user(c, uaddr) != 0))
+			return -EFAULT;
 		uaddr += PAGE_SIZE;
-	}
+	} while (uaddr <= end);
 
 	/* Check whether the range spilled into the next page. */
 	if (((unsigned long)uaddr & PAGE_MASK) ==
 			((unsigned long)end & PAGE_MASK)) {
-		ret = __get_user(c, end);
-		(void)c;
+		return __get_user(c, end);
 	}
 
-	return ret;
+	return 0;
 }
 
 int add_to_page_cache_locked(struct page *page, struct address_space *mapping,

Quite frankly, I think it is access_ok() that should be fixed for s390.

A wrapping user access is *not* ok, not even if kernel and user memory are separate.

It is insane to make fault_in_multipages..() return EFAULT if a normal wrapping user access wouldn't. So the fix is not to change fault_in_multipage_xyz, but to make sure any op that tries to wrap will properly return EFAULT.

So I really think that we should just say "a no-op access_ok() is a buggy access_ok()", and fix the problem at the source, rather than make excuses for it in some random place.

A quick look seems to say that s390 and no-mmu ARM are the only affected cases, but maybe I missed something.

Linus

Not the point. Of course it *would* fail; the problem is that the loop that would ping each page is never executed. What happens is
while (uaddr <= end)
touch uaddr
uaddr += PAGE_SIZE
if uaddr and end point to different pages
ping end

What happens if uaddr is greater than end, thanks to wraparound? Right, we skip the loop entirely and all we do is one ping of the end. Which might very well succeed, leaving us with false positive.

You're missing the point.

If "access_ok()" is fine with wrapping, then some otehr system call that wraps will successfully do a memcpy_from/to_user() with a wrapped address (and the proper mappings). Which is completely bogus.

So access_ok() should be fixed regardless. An access_ok() that accepts a wrapping address is a bug. End of story.

And once that bug is fixed, the fault_in_multipages..() issue is moot. So it shouldn't be an issue.

Linus

What proper mappings? If you manage to mmap something at (void*)-PAGE_SIZE, you are very deep in trouble regardless. We use IS_ERR() on userland pointers and any architecture where that would be possible would be confused as hell.

The testcase here is uaddr = (void *)-1, len = (unsigned long)valid_addr + 2. If it tried to do __put_user(uaddr, 0) it would immediately fail, just as __copy_to_user(uaddr, len); the problem is, that call will only do __put_user(valid_addr, 0) and succeed.

Again, if get_user/put_user/copy_{to,from}_user() anywhere near ERR_PTR(...) would succeed, we'd get trouble without any wraparounds. That page should be absent, and it really is. In all cases, s390 included. Wraparound is irrelevant here. The reason why it got spotted was persistent failure of copy_{to,from}_user after successful fault-ins.

PS: s390 is far from the only such architecture - at least m68k, parisc and sparc64 are the same way.

Sure, we can make all of them check for wraparounds, but what's the point, when actual attempts to copy to/from such range will fail anyway and for absolute majority of the calls the check will do nothing. What's the point?

Note that we need to compare uaddr and end in these loops anyway, so we are not going to save anything there...

[ Sorry, AFK there for a while ]

Ack, you've convinced me. Will apply the patch.

Linus

BTW, folks, there's an interesting part of that story in commit message of af2e84:
-----------------------------------------------------------------------
pagemap.h: fix warning about possibly used before init var

Commit f56f821feb7b ("mm: extend prefault helpers to fault in more than PAGE_SIZE") added in the new functions: fault_in_multipages_writeable() and fault_in_multipages_readable().

However, we currently see:

include/linux/pagemap.h:492: warning: 'ret' may be used uninitialized in this function include/linux/pagemap.h:492: note: 'ret' was declared here

Unlike a lot of gcc nags, this one appears somewhat legit. i.e. passing in an invalid negative value of "size" does make it look like all the conditionals in there would be bypassed and the uninitialized value would be returned.
-----------------------------------------------------------------------

It wasn't just "somewhat" legit - it was entirely accurate and pointing to real bug. The real reason why the loop could be bypassed wasn't the negative 'size' (that really couldn't happen), it was the possibility of wraparound. Which had been masked on most of the architectures by checks that had been pretty far upstream from that function (entire range passing access_ok()) and architecture-dependent on top of that. Some architectures would always prevent wraparounds in access_ok() (x86, for example), some would only do so outside of set_fd(KERNEL_DS) (where the caller is responsible for validity of addresses and lengths) and some did not prevent wraparounds on access_ok() level at all. IOW, it was OK on a lot of architectures, but only for rather subtle reasons. Making ret initialized to 0 did make cc(1) STFU, but didn't fix the bug...

It was used only in intel-specific drivers (i915 is x86-only), arm-specific one (armada) and in NTFS; the last one isn't arch-specific, but I doubt that anyone has really mounted NTFS on e.g. sparc, let alone hit it with LTP or fuzzing there. Multipage path in BTRFS write would benefit from it, and BTRFS probably did get tested on sparc and/or s390, but BTRFS kept using plain fault-in (and paid for that with more frequent fallbacks to singlepage codepath). So it went unnoticed, until it did show up on a path from generic_perform_write()...