LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] iter revert problems
@ 2021-08-09 11:52 Pavel Begunkov
  2021-08-09 11:52 ` [PATCH 1/2] iov_iter: mark truncated iters Pavel Begunkov
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-09 11:52 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, Jens Axboe, io-uring
  Cc: linux-kernel, 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.

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

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

-- 
2.32.0


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

* [PATCH 1/2] iov_iter: mark truncated iters
  2021-08-09 11:52 [PATCH 0/2] iter revert problems Pavel Begunkov
@ 2021-08-09 11:52 ` Pavel Begunkov
  2021-08-09 11:52 ` [PATCH 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
  2021-08-09 15:52 ` [PATCH 0/2] iter revert problems Al Viro
  2 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-09 11:52 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, Jens Axboe, io-uring
  Cc: linux-kernel, 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] 6+ messages in thread

* [PATCH 2/2] io_uring: don't retry with truncated iter
  2021-08-09 11:52 [PATCH 0/2] iter revert problems Pavel Begunkov
  2021-08-09 11:52 ` [PATCH 1/2] iov_iter: mark truncated iters Pavel Begunkov
@ 2021-08-09 11:52 ` Pavel Begunkov
  2021-08-09 15:52 ` [PATCH 0/2] iter revert problems Al Viro
  2 siblings, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-09 11:52 UTC (permalink / raw)
  To: Alexander Viro, linux-fsdevel, Jens Axboe, io-uring
  Cc: linux-kernel, 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: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Reported-by: Palash Oswal <oswalpalash@gmail.com>
Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bf548af0426c..ebf467e0cb0f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2425,6 +2425,8 @@ static bool io_resubmit_prep(struct io_kiocb *req)
 
 	if (!rw)
 		return !io_req_prep_async(req);
+	if (rw->iter.truncated)
+		return false;
 	/* may have left rw->iter inconsistent on -EIOCBQUEUED */
 	iov_iter_revert(&rw->iter, req->result - iov_iter_count(&rw->iter));
 	return true;
@@ -3316,6 +3318,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 (iter->truncated)
+			goto done;
 		/* some cases will consume bytes even on error returns */
 		iov_iter_revert(iter, io_size - iov_iter_count(iter));
 		ret = 0;
@@ -3455,6 +3459,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		kiocb_done(kiocb, ret2, issue_flags);
 	} else {
 copy_iov:
+		if (iter->truncated)
+			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] 6+ messages in thread

* Re: [PATCH 0/2] iter revert problems
  2021-08-09 11:52 [PATCH 0/2] iter revert problems Pavel Begunkov
  2021-08-09 11:52 ` [PATCH 1/2] iov_iter: mark truncated iters Pavel Begunkov
  2021-08-09 11:52 ` [PATCH 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
@ 2021-08-09 15:52 ` Al Viro
  2021-08-09 18:56   ` Pavel Begunkov
  2021-08-10  8:47   ` David Laight
  2 siblings, 2 replies; 6+ messages in thread
From: Al Viro @ 2021-08-09 15:52 UTC (permalink / raw)
  To: Pavel Begunkov; +Cc: linux-fsdevel, Jens Axboe, io-uring, linux-kernel

On Mon, Aug 09, 2021 at 12:52:35PM +0100, 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.

	Umm...  Won't that screw the cases where we *are* doing proper
reexpands?  AFAICS, with your patches that flag doesn't go away once
it had been set...

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

* Re: [PATCH 0/2] iter revert problems
  2021-08-09 15:52 ` [PATCH 0/2] iter revert problems Al Viro
@ 2021-08-09 18:56   ` Pavel Begunkov
  2021-08-10  8:47   ` David Laight
  1 sibling, 0 replies; 6+ messages in thread
From: Pavel Begunkov @ 2021-08-09 18:56 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, Jens Axboe, io-uring, linux-kernel

On 8/9/21 4:52 PM, Al Viro wrote:
> On Mon, Aug 09, 2021 at 12:52:35PM +0100, 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.
> 
> 	Umm...  Won't that screw the cases where we *are* doing proper
> reexpands?  AFAICS, with your patches that flag doesn't go away once
> it had been set...

In general, the userspace should already expecting and retrying on
EAGAIN, and it seems to me, truncates should be rare enough to not
care much about performance. However, it'd better to be more careful
with nowait attempts.

For instance, we can avoid failing reexpanded and reverted iters.

if (i->truncated && iov_iter_count(i) != orig_size)
	// fail;

Or even re-import iov+iter, if still in the right context.


Al, is that viable to you on the iov side?

-- 
Pavel Begunkov

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

* RE: [PATCH 0/2] iter revert problems
  2021-08-09 15:52 ` [PATCH 0/2] iter revert problems Al Viro
  2021-08-09 18:56   ` Pavel Begunkov
@ 2021-08-10  8:47   ` David Laight
  1 sibling, 0 replies; 6+ messages in thread
From: David Laight @ 2021-08-10  8:47 UTC (permalink / raw)
  To: 'Al Viro', Pavel Begunkov
  Cc: linux-fsdevel, Jens Axboe, io-uring, linux-kernel

From: Al Viro
> Sent: 09 August 2021 16:53
> 
> On Mon, Aug 09, 2021 at 12:52:35PM +0100, 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.
> 
> 	Umm...  Won't that screw the cases where we *are* doing proper
> reexpands?  AFAICS, with your patches that flag doesn't go away once
> it had been set...

From what I remember the pointer into the iov[] gets incremented
as it is processed - which makes 'backing up' hard.
The caller also has to remember the original pointer because
it might point to kmalloced memory.

So if the 'iter' always contained a pointer to the base of the iov[]
then various bits of code could be simplified.

Another useful change would be to embed the short iov_cache[8]
inside 'iter'.
Almost all the callers allocate both together (usually on stack)
so the stack use won't change.
I have local patches for most of this (somewhere) but the io_uring
changes start being non-trivial.

	David

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


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

end of thread, other threads:[~2021-08-10  8:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 11:52 [PATCH 0/2] iter revert problems Pavel Begunkov
2021-08-09 11:52 ` [PATCH 1/2] iov_iter: mark truncated iters Pavel Begunkov
2021-08-09 11:52 ` [PATCH 2/2] io_uring: don't retry with truncated iter Pavel Begunkov
2021-08-09 15:52 ` [PATCH 0/2] iter revert problems Al Viro
2021-08-09 18:56   ` Pavel Begunkov
2021-08-10  8:47   ` 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).