[0/2] iter revert problems

Message

Pavel Begunkov Aug. 9, 2021, 11:52 a.m. UTC

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(-)

Comments

Al Viro Aug. 9, 2021, 3:52 p.m. UTC | #1

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

Pavel Begunkov Aug. 9, 2021, 6:56 p.m. UTC | #2

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?

David Laight Aug. 10, 2021, 8:47 a.m. UTC | #3

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)