On Thu, Sep 29, 2016 at 10:53:55PM +0200, Miklos Szeredi wrote:
> The EFAULT logic seems to be missing across the board. And callers
> don't expect a zero return value. Most will loop indefinitely.
Nope. copy_page_to_iter() *never* returns -EFAULT. Including the iovec
one - check copy_page_to_iter_iovec(). Any caller that does not expect
a zero return value from that primitive is a bug, triggerable as soon as
you feed it an iovec with NULL ->iov_base.
> Again, unexpected zero return if this is the first page. Should
> return -ENOMEM? Some callers only expect -EFAULT, though.
For copy_to_iter() and zero_iter() it's definitely "return zero". For
get_pages... Hell knows; those probably ought to return -EFAULT, but
I'll need to look some more at the callers. It should end up triggering
a short read as the end result (or, as usual, EFAULT on zero-length read).
> > + /* take it relative to the beginning of buffer */
> > + size += off - pipe->bufs[idx].offset;
> > + while (1) {
> > + buf = &pipe->bufs[idx];
> > + if (size > buf->len) {
> > + size -= buf->len;
> > + idx = next_idx(idx, pipe);
> > + off = 0;
>
> off is unused and reassigned before breaking out of the loop.
True.
> [...]
>
> > + if (unlikely(i->type & ITER_PIPE)) {
> > + struct pipe_inode_info *pipe = i->pipe;
> > + size_t off;
> > + int idx;
> > +
> > + if (!sanity(i))
> > + return 0;
> > +
> > + data_start(i, &idx, &off);
> > + /* some of this one + all after this one */
> > + npages = ((pipe->curbuf - idx - 1) & (pipe->buffers - 1)) +
> > 1;
>
> It's supposed to take i->count into account, no? And that calculation
> will result in really funny things if the pipe is full. And we can't
> return -EFAULT here, since that's not expected by callers...
It should look at i->count, in principle. OTOH, overestimating the amount
is not really a problem for possible users of such iov_iter. I'll look
into that.
|