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