On Wed, Nov 04, 2020 at 09:27:33AM +0100, Christoph Hellwig wrote:

>  ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t 
> *ppos)
>  {
> -     struct seq_file *m = file->private_data;
> +     struct iovec iov = { .iov_base = buf, .iov_len = size};
> +     struct kiocb kiocb;
> +     struct iov_iter iter;
> +     ssize_t ret;
> +
> +     init_sync_kiocb(&kiocb, file);
> +     iov_iter_init(&iter, READ, &iov, 1, size);
> +
> +     kiocb.ki_pos = *ppos;
> +     ret = seq_read_iter(&kiocb, &iter);
> +     *ppos = kiocb.ki_pos;
> +     return ret;
> +}
> +EXPORT_SYMBOL(seq_read);

This is basically an open-coded copy of new_sync_read()...

>       if (m->count) {
>               n = min(m->count, size);
> -             err = copy_to_user(buf, m->buf + m->from, n);
> -             if (err)
> +             if (copy_to_iter(m->buf + m->from, n, iter) != n)
>                       goto Efault;
>               m->count -= n;
>               m->from += n;
>               size -= n;
> -             buf += n;
>               copied += n;
>               if (!size)
>                       goto Done;

>       n = min(m->count, size);
> -     err = copy_to_user(buf, m->buf, n);
> -     if (err)
> +     if (copy_to_iter(m->buf, n, iter) != n)
>               goto Efault;

This is actually broken from generic_file_splice_read() POV; if you've
already emitted something, you will end up with more data spewed into
pipe than you report to caller.  You want something similar to
copy_to_iter_full() here, with iterator _not_ advanced in case of
failure.  The first call is not an issue (you have no data copied
yet, so you'll end up with -EFAULT, aka "discard everything you've
put there and return -EAGAIN"), but the second really is a problem.

BTW, other ->read_iter() instances might need to be careful with that
pattern as well; drivers/gpu/drm/drm_dp_aux_dev.c:auxdev_read_iter()
would appear to have the same problem.

<greps some more>
                if (unlikely(iov_iter_is_pipe(iter))) {
                        void *addr = kmap_atomic(page);

                        written = copy_to_iter(addr, copy, iter);
                        kunmap_atomic(addr);   
                } else
in fs/cifs/file.c looks... interesting, considering the fact that
copy_to_iter() for pipe destination might very well have to do
allocations.  With GFP_USER.  Under kmap_atomic()...

Note that we have this:
static inline int copy_linear_skb(struct sk_buff *skb, int len, int off,
                                  struct iov_iter *to)
{
        int n;

        n = copy_to_iter(skb->data + off, len, to);
        if (n == len)
                return 0;

        iov_iter_revert(to, n);
        return -EFAULT;
}
i.e. the same "do not advance on short copy" kind of thing.

AFAICS, not all callers want that semantics, but I think it's worth
a new primitive.  I'm not saying it should be a prereq for your
series, but either that or an explicit iov_iter_revert() is needed.

Reply via email to