[ add Tony and Boris ]
Al Viro wrote:
> [commit in question sits in vfs.git#fixes]
>
> Unlike other copying operations on ITER_PIPE, copy_mc_to_iter() can
> result in a short copy. In that case we need to trim the unused
> buffers, as well as the length of partially filled one - it's not
> enough to set ->head, ->iov_offset and ->count to reflect how
> much had we copied. Not hard to fix, fortunately...
>
> I'd put a helper (pipe_discard_from(pipe, head)) into pipe_fs_i.h,
> rather than iov_iter.c - it has nothing to do with iov_iter and
> having it will allow us to avoid an ugly kludge in fs/splice.c.
> We could put it into lib/iov_iter.c for now and move it later,
> but I don't see the point going that way...
Apologies for the delay in responding (reworking my email workflow after
a loss of Gmail access for my intel.com address). This looks good to me:
Acked-by: Dan Williams <dan.j.williams@xxxxxxxxx>
...and I also share the concern from Linus about the lack of testing
this gets outside of systems with the necessary hardware/firmware to do
error injection testing.
Boris and I had agreed to remove some software error injection machinery
for copy_mc_* in commit 3adb776384f2 ("x86, libnvdimm/test: Remove
COPY_MC_TEST"). Is there an appetite to see some of that return and
write a regression test for this bug?
>
> Fixes: ca146f6f091e "lib/iov_iter: Fix pipe handling in _copy_to_iter_mcsafe()"
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index cb0fd633a610..4ea496924106 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -229,6 +229,15 @@ static inline bool pipe_buf_try_steal(struct pipe_inode_info *pipe,
> return buf->ops->try_steal(pipe, buf);
> }
>
> +static inline void pipe_discard_from(struct pipe_inode_info *pipe,
> + unsigned int old_head)
> +{
> + unsigned int mask = pipe->ring_size - 1;
> +
> + while (pipe->head > old_head)
> + pipe_buf_release(pipe, &pipe->bufs[--pipe->head & mask]);
> +}
> +
> /* Differs from PIPE_BUF in that PIPE_SIZE is the length of the actual
> memory allocation, whereas PIPE_BUF makes atomicity guarantees. */
> #define PIPE_SIZE PAGE_SIZE
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 0b64695ab632..2bf20b48a04a 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -689,6 +689,7 @@ static size_t copy_mc_pipe_to_iter(const void *addr, size_t bytes,
> struct pipe_inode_info *pipe = i->pipe;
> unsigned int p_mask = pipe->ring_size - 1;
> unsigned int i_head;
> + unsigned int valid = pipe->head;
> size_t n, off, xfer = 0;
>
> if (!sanity(i))
> @@ -702,11 +703,17 @@ static size_t copy_mc_pipe_to_iter(const void *addr, size_t bytes,
> rem = copy_mc_to_kernel(p + off, addr + xfer, chunk);
> chunk -= rem;
> kunmap_local(p);
> - i->head = i_head;
> - i->iov_offset = off + chunk;
> - xfer += chunk;
> - if (rem)
> + if (chunk) {
> + i->head = i_head;
> + i->iov_offset = off + chunk;
> + xfer += chunk;
> + valid = i_head + 1;
> + }
> + if (rem) {
> + pipe->bufs[i_head & p_mask].len -= rem;
> + pipe_discard_from(pipe, valid);
> break;
> + }
> n -= chunk;
> off = 0;
> i_head++;
