Re: [PATCH 1/6] seq_file: add seq_read_iter

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Nov 11, 2020 at 09:54:12AM -0800, Linus Torvalds wrote:
> On Tue, Nov 10, 2020 at 3:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Any objections to the following?
> 
> Well, I don't _object_, but I find it ugly.
> 
> And I think both the old and the "fixed" code is wrong when an EFAULT
> happens in the middle.
> 
> Yes, we can just return EFAULT. But for read() and write() we really
> try to do the proper partial returns in other places, why not here?
> 
> IOW, why isn't the proper fix just something like this:
> 
>     diff --git a/fs/seq_file.c b/fs/seq_file.c
>     index 3b20e21604e7..ecc6909b71f5 100644
>     --- a/fs/seq_file.c
>     +++ b/fs/seq_file.c
>     @@ -209,7 +209,8 @@ ssize_t seq_read_iter(struct kiocb *iocb,
> struct iov_iter *iter)
>         /* if not empty - flush it first */
>         if (m->count) {
>                 n = min(m->count, size);
>     -           if (copy_to_iter(m->buf + m->from, n, iter) != n)
>     +           n = copy_to_iter(m->buf + m->from, n, iter);
>     +           if (!n)
>                         goto Efault;
>                 m->count -= n;
>                 m->from += n;
> 
> which should get the "efault in the middle" case roughly right (ie the
> usual "exact byte alignment and page crosser" caveats apply).

Look at the loop after that one, specifically the "it doesn't fit,
allocate a bigger one" part:
                kvfree(m->buf);
                m->count = 0;
                m->buf = seq_buf_alloc(m->size <<= 1);
It really depends upon having m->buf empty at the beginning of
the loop.  Your variant will lose the data, unless we copy the
"old" part of buffer (from before the ->show()) into the
larger one.

That can be done, but I would rather go with
		n = copy_to_iter(m->buf + m->from, m->count, iter);
		m->count -= n;
		m->from += n;
                copied += n;
                if (!size)
                        goto Done;
		if (m->count)
			goto Efault;
if we do it that way.  Let me see if I can cook something
reasonable along those lines...