LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2 0/2] iter revert problems
@ 2021-08-12 20:40 Pavel Begunkov
  2021-08-12 20:40 ` [PATCH v2 1/2] iov_iter: mark truncated iters Pavel Begunkov
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Pavel Begunkov @ 2021-08-12 20:40 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Alexander Viro, linux-fsdevel
  Cc: Palash Oswal, Sudip Mukherjee, linux-kernel,
	syzbot+9671693590ef5aad8953, asml.silence

For the bug description see 2/2. As mentioned there the current problems
is because of generic_write_checks(), but there was also a similar case
fixed in 5.12, which should have been triggerable by normal
write(2)/read(2) and others.

It may be better to enforce reexpands as a long term solution, but for
now this patchset is quickier and easier to backport.

v2: don't fail it has been justly fully reverted

Pavel Begunkov (2):
  iov_iter: mark truncated iters
  io_uring: don't retry with truncated iter

 fs/io_uring.c       | 16 ++++++++++++++++
 include/linux/uio.h |  5 ++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] iov_iter: mark truncated iters
  2021-08-12 20:40 [PATCH v2 0/2] iter revert problems Pavel Begunkov
@ 2021-08-12 20:40 ` Pavel Begunkov
  2021-08-12 20:40 ` [PATCH v2 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Pavel Begunkov @ 2021-08-12 20:40 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Alexander Viro, linux-fsdevel
  Cc: Palash Oswal, Sudip Mukherjee, linux-kernel,
	syzbot+9671693590ef5aad8953, asml.silence

Track if an iterator has ever been truncated. This will be used to
mitigate revert-truncate problems.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/uio.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 82c3c3e819e0..61b8d312d13a 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -30,6 +30,7 @@ enum iter_type {
 struct iov_iter {
 	u8 iter_type;
 	bool data_source;
+	bool truncated;
 	size_t iov_offset;
 	size_t count;
 	union {
@@ -254,8 +255,10 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
 	 * conversion in assignement is by definition greater than all
 	 * values of size_t, including old i->count.
 	 */
-	if (i->count > count)
+	if (i->count > count) {
 		i->count = count;
+		i->truncated = true;
+	}
 }
 
 /*
-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] io_uring: don't retry with truncated iter
  2021-08-12 20:40 [PATCH v2 0/2] iter revert problems Pavel Begunkov
  2021-08-12 20:40 ` [PATCH v2 1/2] iov_iter: mark truncated iters Pavel Begunkov
@ 2021-08-12 20:40 ` Pavel Begunkov
  2021-08-16 15:35 ` [PATCH v2 0/2] iter revert problems Jens Axboe
  2021-08-21 14:24 ` Pavel Begunkov
  3 siblings, 0 replies; 8+ messages in thread
From: Pavel Begunkov @ 2021-08-12 20:40 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Alexander Viro, linux-fsdevel
  Cc: Palash Oswal, Sudip Mukherjee, linux-kernel,
	syzbot+9671693590ef5aad8953, asml.silence

[   74.211232] BUG: KASAN: stack-out-of-bounds in iov_iter_revert+0x809/0x900
[   74.212778] Read of size 8 at addr ffff888025dc78b8 by task
syz-executor.0/828
[   74.214756] CPU: 0 PID: 828 Comm: syz-executor.0 Not tainted
5.14.0-rc3-next-20210730 #1
[   74.216525] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[   74.219033] Call Trace:
[   74.219683]  dump_stack_lvl+0x8b/0xb3
[   74.220706]  print_address_description.constprop.0+0x1f/0x140
[   74.224226]  kasan_report.cold+0x7f/0x11b
[   74.226085]  iov_iter_revert+0x809/0x900
[   74.227960]  io_write+0x57d/0xe40
[   74.232647]  io_issue_sqe+0x4da/0x6a80
[   74.242578]  __io_queue_sqe+0x1ac/0xe60
[   74.245358]  io_submit_sqes+0x3f6e/0x76a0
[   74.248207]  __do_sys_io_uring_enter+0x90c/0x1a20
[   74.257167]  do_syscall_64+0x3b/0x90
[   74.257984]  entry_SYSCALL_64_after_hwframe+0x44/0xae

old_size = iov_iter_count();
...
iov_iter_revert(old_size - iov_iter_count());

If iov_iter_revert() is done base on the initial size as above, and the
iter is truncated and not reexpanded in the middle, it miscalculates
borders causing problems. This trace is due to no one reexpanding after
generic_write_checks().

Avoid reverting truncated iterators, so io_uring would fail requests
with EAGAIN instead of retrying them.

Cc: stable@vger.kernel.org
Reported-by: Palash Oswal <oswalpalash@gmail.com>
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Reported-and-tested-by: syzbot+9671693590ef5aad8953@syzkaller.appspotmail.com
Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index efd818419014..2e168051262d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2462,6 +2462,16 @@ static void kiocb_end_write(struct io_kiocb *req)
 	}
 }
 
+static inline bool io_check_truncated(struct iov_iter *i, size_t len)
+{
+	if (unlikely(i->truncated)) {
+		if (iov_iter_count(i) != len)
+			return false;
+		i->truncated = false;
+	}
+	return true;
+}
+
 #ifdef CONFIG_BLOCK
 static bool io_resubmit_prep(struct io_kiocb *req)
 {
@@ -2469,6 +2479,8 @@ static bool io_resubmit_prep(struct io_kiocb *req)
 
 	if (!rw)
 		return !io_req_prep_async(req);
+	if (!io_check_truncated(&rw->iter, req->result))
+		return false;
 	/* may have left rw->iter inconsistent on -EIOCBQUEUED */
 	iov_iter_revert(&rw->iter, req->result - iov_iter_count(&rw->iter));
 	return true;
@@ -3328,6 +3340,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 		/* no retry on NONBLOCK nor RWF_NOWAIT */
 		if (req->flags & REQ_F_NOWAIT)
 			goto done;
+		if (!io_check_truncated(iter, io_size))
+			goto done;
 		/* some cases will consume bytes even on error returns */
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = 0;
@@ -3467,6 +3481,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		kiocb_done(kiocb, ret2, issue_flags);
 	} else {
 copy_iov:
+		if (!io_check_truncated(iter, io_size))
+			goto done;
 		/* some cases will consume bytes even on error returns */
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] iter revert problems
  2021-08-12 20:40 [PATCH v2 0/2] iter revert problems Pavel Begunkov
  2021-08-12 20:40 ` [PATCH v2 1/2] iov_iter: mark truncated iters Pavel Begunkov
  2021-08-12 20:40 ` [PATCH v2 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
@ 2021-08-16 15:35 ` Jens Axboe
  2021-08-21 14:24 ` Pavel Begunkov
  3 siblings, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2021-08-16 15:35 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring, Alexander Viro, linux-fsdevel
  Cc: Palash Oswal, Sudip Mukherjee, linux-kernel, syzbot+9671693590ef5aad8953

On 8/12/21 2:40 PM, Pavel Begunkov wrote:
> For the bug description see 2/2. As mentioned there the current problems
> is because of generic_write_checks(), but there was also a similar case
> fixed in 5.12, which should have been triggerable by normal
> write(2)/read(2) and others.
> 
> It may be better to enforce reexpands as a long term solution, but for
> now this patchset is quickier and easier to backport.
> 
> v2: don't fail it has been justly fully reverted

Al, what do you think of this approach? It'll fix the issue, but might be
cleaner to have iov->truncated actually track the truncated size. That'd
make it a more complete solution, at the expense of bloat iov_iter which
this version will not.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] iter revert problems
  2021-08-12 20:40 [PATCH v2 0/2] iter revert problems Pavel Begunkov
                   ` (2 preceding siblings ...)
  2021-08-16 15:35 ` [PATCH v2 0/2] iter revert problems Jens Axboe
@ 2021-08-21 14:24 ` Pavel Begunkov
  2021-08-21 22:25   ` Al Viro
  3 siblings, 1 reply; 8+ messages in thread
From: Pavel Begunkov @ 2021-08-21 14:24 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Alexander Viro, linux-fsdevel
  Cc: Palash Oswal, Sudip Mukherjee, linux-kernel, syzbot+9671693590ef5aad8953

On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> For the bug description see 2/2. As mentioned there the current problems
> is because of generic_write_checks(), but there was also a similar case
> fixed in 5.12, which should have been triggerable by normal
> write(2)/read(2) and others.
> 
> It may be better to enforce reexpands as a long term solution, but for
> now this patchset is quickier and easier to backport.

We need to do something with this, hopefully soon.


> v2: don't fail it has been justly fully reverted
> 
> Pavel Begunkov (2):
>   iov_iter: mark truncated iters
>   io_uring: don't retry with truncated iter
> 
>  fs/io_uring.c       | 16 ++++++++++++++++
>  include/linux/uio.h |  5 ++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] iter revert problems
  2021-08-21 14:24 ` Pavel Begunkov
@ 2021-08-21 22:25   ` Al Viro
  2021-08-21 23:13     ` Jens Axboe
  0 siblings, 1 reply; 8+ messages in thread
From: Al Viro @ 2021-08-21 22:25 UTC (permalink / raw)
  To: Pavel Begunkov
  Cc: Jens Axboe, io-uring, linux-fsdevel, Palash Oswal,
	Sudip Mukherjee, linux-kernel, syzbot+9671693590ef5aad8953

On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> > For the bug description see 2/2. As mentioned there the current problems
> > is because of generic_write_checks(), but there was also a similar case
> > fixed in 5.12, which should have been triggerable by normal
> > write(2)/read(2) and others.
> > 
> > It may be better to enforce reexpands as a long term solution, but for
> > now this patchset is quickier and easier to backport.
> 
> We need to do something with this, hopefully soon.

I still don't like that approach ;-/  If anything, I would rather do
something like this, and to hell with one extra word on stack in
several functions; at least that way the semantics is easy to describe.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/io_uring.c b/fs/io_uring.c
index d94fb5835a20..5501f8b3af3b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3420,6 +3420,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	} else {
 copy_iov:
 		/* some cases will consume bytes even on error returns */
+		iov_iter_reexpand(iter, iter->count + iter->truncated);
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
 		return ret ?: -EAGAIN;
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 82c3c3e819e0..5265024e8b90 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -47,6 +47,7 @@ struct iov_iter {
 		};
 		loff_t xarray_start;
 	};
+	size_t truncated;
 };
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
@@ -254,8 +255,10 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
 	 * conversion in assignement is by definition greater than all
 	 * values of size_t, including old i->count.
 	 */
-	if (i->count > count)
+	if (i->count > count) {
+		i->truncated += i->count - count;
 		i->count = count;
+	}
 }
 
 /*
@@ -264,6 +267,7 @@ static inline void iov_iter_truncate(struct iov_iter *i, u64 count)
  */
 static inline void iov_iter_reexpand(struct iov_iter *i, size_t count)
 {
+	i->truncated -= count - i->count;
 	i->count = count;
 }
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] iter revert problems
  2021-08-21 22:25   ` Al Viro
@ 2021-08-21 23:13     ` Jens Axboe
  2021-08-23 12:28       ` David Laight
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2021-08-21 23:13 UTC (permalink / raw)
  To: Al Viro, Pavel Begunkov
  Cc: io-uring, linux-fsdevel, Palash Oswal, Sudip Mukherjee,
	linux-kernel, syzbot+9671693590ef5aad8953

On 8/21/21 4:25 PM, Al Viro wrote:
> On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
>> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
>>> For the bug description see 2/2. As mentioned there the current problems
>>> is because of generic_write_checks(), but there was also a similar case
>>> fixed in 5.12, which should have been triggerable by normal
>>> write(2)/read(2) and others.
>>>
>>> It may be better to enforce reexpands as a long term solution, but for
>>> now this patchset is quickier and easier to backport.
>>
>> We need to do something with this, hopefully soon.
> 
> I still don't like that approach ;-/  If anything, I would rather do
> something like this, and to hell with one extra word on stack in
> several functions; at least that way the semantics is easy to describe.

Pavel suggested this very approach initially as well when we discussed
it, and if you're fine with the extra size_t, it is by far the best way
to get this done and not have a wonky/fragile API.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2 0/2] iter revert problems
  2021-08-21 23:13     ` Jens Axboe
@ 2021-08-23 12:28       ` David Laight
  0 siblings, 0 replies; 8+ messages in thread
From: David Laight @ 2021-08-23 12:28 UTC (permalink / raw)
  To: 'Jens Axboe', Al Viro, Pavel Begunkov
  Cc: io-uring, linux-fsdevel, Palash Oswal, Sudip Mukherjee,
	linux-kernel, syzbot+9671693590ef5aad8953

From: Jens Axboe
> Sent: 22 August 2021 00:14
> 
> On 8/21/21 4:25 PM, Al Viro wrote:
> > On Sat, Aug 21, 2021 at 03:24:28PM +0100, Pavel Begunkov wrote:
> >> On 8/12/21 9:40 PM, Pavel Begunkov wrote:
> >>> For the bug description see 2/2. As mentioned there the current problems
> >>> is because of generic_write_checks(), but there was also a similar case
> >>> fixed in 5.12, which should have been triggerable by normal
> >>> write(2)/read(2) and others.
> >>>
> >>> It may be better to enforce reexpands as a long term solution, but for
> >>> now this patchset is quickier and easier to backport.
> >>
> >> We need to do something with this, hopefully soon.
> >
> > I still don't like that approach ;-/  If anything, I would rather do
> > something like this, and to hell with one extra word on stack in
> > several functions; at least that way the semantics is easy to describe.
> 
> Pavel suggested this very approach initially as well when we discussed
> it, and if you're fine with the extra size_t, it is by far the best way
> to get this done and not have a wonky/fragile API.

All (well maybe almost all) the users of iov_iter have the
short iov[] cache and the pointer to the big iov[] to kfree()
allocated together with the iov_iter structure itself.
These are almost always on stack.

Putting the whole lot together in a single structure would
make the call sequences a lot less complex and wouldn't use
any more stack/data is almost all the cases.

It would also mean that the 'iter' code could always have a pointer
to the base of the original iov[] list.
The lack of which is probably makes the 'revert' code hard?

	David

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-08-23 12:28 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12 20:40 [PATCH v2 0/2] iter revert problems Pavel Begunkov
2021-08-12 20:40 ` [PATCH v2 1/2] iov_iter: mark truncated iters Pavel Begunkov
2021-08-12 20:40 ` [PATCH v2 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
2021-08-16 15:35 ` [PATCH v2 0/2] iter revert problems Jens Axboe
2021-08-21 14:24 ` Pavel Begunkov
2021-08-21 22:25   ` Al Viro
2021-08-21 23:13     ` Jens Axboe
2021-08-23 12:28       ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).