[PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[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

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
v2: use kmap

P.S. this one passes all tests well

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8119cbae4fb6..1a9f34645586 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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;
-- 
2.24.0

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jens Axboe]

On 11/24/19 1:58 AM, Pavel Begunkov wrote:
> 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.

-- 
Jens Axboe

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 1034 bytes --]

On 24/11/2019 20:10, Jens Axboe wrote:
> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>> 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.
> 
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?

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jackie Liu]

> 2019年11月25日 01:52，Pavel Begunkov <asml.silence@gmail.com> 写道：
> 
> On 24/11/2019 20:10, Jens Axboe wrote:
>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>> 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.
>> 
> 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

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jens Axboe]

On 11/24/19 5:43 PM, Jackie Liu wrote:
> 
> 
>> 2019年11月25日 01:52，Pavel Begunkov <asml.silence@gmail.com> 写道：
>>
>> On 24/11/2019 20:10, Jens Axboe wrote:
>>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>>> 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.
>>>
>> 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?

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.

-- 
Jens Axboe

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jackie Liu]

> 2019年11月25日 10:38，Jens Axboe <axboe@kernel.dk> 写道：
> 
> On 11/24/19 5:43 PM, Jackie Liu wrote:
>> 
>> 
>>> 2019年11月25日 01:52，Pavel Begunkov <asml.silence@gmail.com> 写道：
>>> 
>>> On 24/11/2019 20:10, Jens Axboe wrote:
>>>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>>>> 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.
>>>> 
>>> 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?
> 
> 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 <liuyun01@kylinos.cn>

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

--
Jackie Liu

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jens Axboe]

> On Nov 24, 2019, at 8:38 PM, Jackie Liu <jackieliu@byteisland.com> wrote:
> 
> 
> 
>> 2019年11月25日 10:38，Jens Axboe <axboe@kernel.dk> 写道：
>> 
>>> On 11/24/19 5:43 PM, Jackie Liu wrote:
>>> 
>>> 
>>>> 2019年11月25日 01:52，Pavel Begunkov <asml.silence@gmail.com> 写道：
>>>> 
>>>> On 24/11/2019 20:10, Jens Axboe wrote:
>>>>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>>>>> 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.
>>>>> 
>>>> 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?
>> 
>> 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 <liuyun01@kylinos.cn>

Thanks!

> 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

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Pavel Begunkov]

>>> 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.
>>
> 
> For test/stdout is PASS. Tested-by: Jackie Liu <liuyun01@kylinos.cn>
>
Great, thanks!

-- 
Pavel Begunkov

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Jens Axboe]

On 11/24/19 10:52 AM, Pavel Begunkov wrote:
> On 24/11/2019 20:10, Jens Axboe wrote:
>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>> 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.
>>
> 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.

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

-- 
Jens Axboe

Re: [PATCH v2] io_uring: fix dead-hung for non-iter fixed rw

[Pavel Begunkov]

On 11/25/2019 5:37 AM, Jens Axboe wrote:
> On 11/24/19 10:52 AM, Pavel Begunkov wrote:
>> On 24/11/2019 20:10, Jens Axboe wrote:
>>> On 11/24/19 1:58 AM, Pavel Begunkov wrote:
>>>> 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.
>>>
>> 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.
> 
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?

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

-- 
Pavel Begunkov