io_uring: fix dead-hung for non-iter fixed rw

PATCH In review Pavel Begunkov 3 participants 10 messages Nov 24, 2019 – Nov 25, 2019 1 message not in the archive

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Pavel Begunkov

author

2019-11-24 08:58 UTC #1 lore raw

PATCH v2 1/1 io_uring: fix dead-hung for non-iter fixed rw

Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.

io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.

kmap() page by page in this case

Signed-off-by Pavel Begunkov <[email protected]>

---
v2: use kmap

P.S. this one passes all tests well

 fs/io_uring.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

fs/io_uring.c +14 -1

index 8119cbae4fb6..1a9f34645586 100644
@@ -1613,9 +1613,19 @@ static ssize_t loop_rw_iter(int rw, struct file *file, struct kiocb *kiocb,
 		return -EAGAIN;
 
 	while (iov_iter_count(iter)) {
-		struct iovec iovec = iov_iter_iovec(iter);
+		struct iovec iovec;
 		ssize_t nr;
 
+		if (!iov_iter_is_bvec(iter)) {
+			iovec = iov_iter_iovec(iter);
+		} else {
+			/* fixed buffers import bvec */
+			iovec.iov_base = kmap(iter->bvec->bv_page)
+						+ iter->iov_offset;
+			iovec.iov_len = min(iter->count,
+					iter->bvec->bv_len - iter->iov_offset);
+		}
+
 		if (rw == READ) {
 			nr = file->f_op->read(file, iovec.iov_base,
 					      iovec.iov_len, &kiocb->ki_pos);

@@ -1624,6 +1634,9 @@ static ssize_t loop_rw_iter(int rw, struct file *file, struct kiocb *kiocb,
 					       iovec.iov_len, &kiocb->ki_pos);
 		}
 
+		if (iov_iter_is_bvec(iter))
+			kunmap(iter->bvec->bv_page);
+
 		if (nr < 0) {
 			if (!ret)
 				ret = nr;

Signature · 1 line

2.24.0

Jens Axboe

maintainer

2019-11-24 17:10 UTC #2 lore raw

Quoted 8 lines · Pavel Begunkov

Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case

This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2019-11-24 17:52 UTC #3 lore raw

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.

Quoted 9 lines

Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.

The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...

#define access_ok(...) __range_not_ok(addr, user_addr_max());

BTW, is there anybody testing it for non x86-64 arch?

Signature · 1 line

Pavel Begunkov

Jackie Liu

2019-11-25 00:43 UTC #4 lore raw

Quoted 19 lines

2019年11月25日 01:52，Pavel Begunkov <[email protected]> 写道：
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.

Quoted 9 lines

The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...
#define access_ok(...) __range_not_ok(addr, user_addr_max());
BTW, is there anybody testing it for non x86-64 arch?

I have some aarch64 platform, What test do you want me to do?

--
Jackie Liu

Jens Axboe

maintainer

2019-11-25 02:38 UTC #5 lore raw

Quoted 24 lines · Jackie Liu

Quoted 24 lines
2019年11月25日 01:52，Pavel Begunkov <[email protected]> 写道：
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.
The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...

Quoted 6 lines

Quoted 4 lines
#define access_ok(...) __range_not_ok(addr, user_addr_max());
BTW, is there anybody testing it for non x86-64 arch?
I have some aarch64 platform, What test do you want me to do?

A basic one to try would be:

axboe@x1:/home/axboe/git/liburing $ test/stdout
This is a pipe test
This is a fixed pipe test

But in general I'd just run make runtests in the liburing directory and go through all of them.

Signature · 1 line

Jens Axboe

Jackie Liu

2019-11-25 03:38 UTC #6 lore raw

Quoted 36 lines

2019年11月25日 10:38，Jens Axboe <[email protected]> 写道：
Quoted 24 lines · Jackie Liu
Quoted 24 lines
2019年11月25日 01:52，Pavel Begunkov <[email protected]> 写道：
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.
The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...
Quoted 6 lines
Quoted 4 lines
#define access_ok(...) __range_not_ok(addr, user_addr_max());
BTW, is there anybody testing it for non x86-64 arch?
I have some aarch64 platform, What test do you want me to do?

Quoted 9 lines

A basic one to try would be:
axboe@x1:/home/axboe/git/liburing $ test/stdout
This is a pipe test
This is a fixed pipe test
But in general I'd just run make runtests in the liburing directory and go through all of them.

For test/stdout is PASS. Tested-by: Jackie Liu <[email protected]>

But test/accept-link and test/fixed-link failed in for-5.5/io_uring-post branch. that is expect?

--
Jackie Liu

Jens Axboe

maintainer

2019-11-25 03:47 UTC #7 lore raw

Quoted 45 lines · Jackie Liu

Quoted 40 lines
Quoted 25 lines · 2019年11月25日 10:38，Jens Axboe <[email protected]> 写道：
Quoted 22 lines · Jackie Liu
2019年11月25日 01:52，Pavel Begunkov <[email protected]> 写道：
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.
The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...
Quoted 6 lines
Quoted 4 lines
#define access_ok(...) __range_not_ok(addr, user_addr_max());
BTW, is there anybody testing it for non x86-64 arch?
I have some aarch64 platform, What test do you want me to do?
A basic one to try would be:
axboe@x1:/home/axboe/git/liburing $ test/stdout
This is a pipe test
This is a fixed pipe test

Quoted 5 lines

Quoted 3 lines
But in general I'd just run make runtests in the liburing directory and go through all of them.
For test/stdout is PASS. Tested-by: Jackie Liu <[email protected]>

Thanks!

Quoted 2 lines

But test/accept-link and test/fixed-link failed in for-5.5/io_uring-post branch. that is expect?

Yes, the fix for that is in 5.4, didn’t merge it into the 5.5 branch. It’ll pass once Linus pulls the 5.5 bits and the branches merge.

—
Jens Axboe

Pavel Begunkov

author

2019-11-25 10:12 UTC #8 lore raw

In reply to Jackie Liu

Quoted 8 lines

Quoted 8 lines
Quoted 1 line
I have some aarch64 platform, What test do you want me to do?
A basic one to try would be:
axboe@x1:/home/axboe/git/liburing $ test/stdout
This is a pipe test
This is a fixed pipe test

Quoted 6 lines

Quoted 3 lines
But in general I'd just run make runtests in the liburing directory and go through all of them.
For test/stdout is PASS. Tested-by: Jackie Liu <[email protected]>

Great, thanks!

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2019-11-25 02:37 UTC #9 lore raw

In reply to Pavel Begunkov

Quoted 15 lines · Pavel Begunkov

Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.

Quoted 6 lines

The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...
#define access_ok(...) __range_not_ok(addr, user_addr_max());

They are user pages! So this should be totally fine. The only difference is that we have them pre-mapped. But it's not like we're pretending these kernel pages are user pages, and hence access_ok() should be totally happy with them.

Quoted 1 line

BTW, is there anybody testing it for non x86-64 arch?

Would be nice, I've mostly failed at getting other archs interested enough to actually make hardware available. Which seems pretty lame, but only so much I can do there. There _shouldn't_ be anything arch specific, but it would be great to have archs with eg weaker ordering as part of the regular test arsenal.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2019-11-25 10:46 UTC #10 lore raw

Quoted 21 lines · Jens Axboe

Quoted 20 lines · Pavel Begunkov
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
Read/write requests to devices without implemented read/write_iter using fixed buffers causes general protection fault, which totally hangs a machine.
Quoted 9 lines
Quoted 4 lines
io_import_fixed() initialises iov_iter with bvec, but loop_rw_iter() accesses it as iovec, so dereferencing random address.
kmap() page by page in this case
This looks good to me, much cleaner/simpler. I've added a few pipe fixed buffer tests to liburing as well. Didn't crash for me, but obvious garbage coming out. I've flagged this for stable as well.
The problem I have is that __user pointer is meant to be checked for not being a kernel address. I suspect, it could fail in some device, which double checks the pointer after vfs (e.g. using access_ok()). Am I wrong? Not a fault at least...

Quoted 7 lines

Quoted 1 line
#define access_ok(...) __range_not_ok(addr, user_addr_max());
They are user pages! So this should be totally fine. The only difference is that we have them pre-mapped. But it's not like we're pretending these kernel pages are user pages, and hence access_ok() should be totally happy with them.

Good, if you say so, but I'll take the liberty of asking a little bit more :)

Yes, they are user pages, but that's rather about used virtual addresses. Having virtual address space separation (e.g. [0-n): user's virtual address space part, [n-m): kernel's one), I'd expect __user ptr to be checked to be bounded by [0-n). E.g. copy_to_user() obviously shouldn't write into kernel's addresses. And I also assume that kmap() maps into [n-m), at least because the kernel may want to allocate and kmap() pages, and use them internally.

And that's why I thought it may fail.
E.g. vfs_read_sys((__user void*)kmap()) _should_ fail. Where is my mistake?

Quoted 8 lines

Quoted 1 line
BTW, is there anybody testing it for non x86-64 arch?
Would be nice, I've mostly failed at getting other archs interested enough to actually make hardware available. Which seems pretty lame, but only so much I can do there. There _shouldn't_ be anything arch specific, but it would be great to have archs with eg weaker ordering as part of the regular test arsenal.

Yeah, that's the point. It probably needs some use in Android to turn the things over.

Signature · 1 line

Pavel Begunkov