Re: WARNING in csum_and_copy_to_iter

[Slavomir Kaslev]

On Sun, Nov 25, 2018 at 3:52 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Sat, Nov 24, 2018 at 09:44:36PM +0000, Al Viro wrote:
>
> > No point, IMO - the fix isn't hard and bisect hazard created by the whole thing
> > is both mild (spurious WARN() in case that used to fail anyway) _and_ won't
> > disappear from reverting, obviously.  I'll post a fix later tonight...
>
> FWIW, I think the following ought to work; it's obviously a pair of commits
> (introduction of convenience helper/switch to its use + csum_and_copy_to_iter()
> for ITER_PIPE), as well as commit message, etc., but I would really appreciate
> if folks gave it a look _and_ a beating.

Tested the patch in qemu, splice reading from udp and vsock sockets (with
https://github.com/skaslev/thru), and it seems to work great.

No warnings or suspicious messages in dmesg with kernel config similar to what
syzbot is using
https://github.com/google/syzkaller/blob/master/docs/linux/kernel_configs.md

> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 7ebccb5c1637..621984743268 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -560,6 +560,44 @@ static size_t copy_pipe_to_iter(const void *addr, size_t bytes,
>         return bytes;
>  }
>
> +static __wsum csum_and_memcpy(void *to, const void *from, size_t len,
> +                             __wsum sum, size_t off)
> +{
> +       __wsum next = csum_partial_copy_nocheck(from, to, len, 0);
> +       return csum_block_add(sum, next, off);
> +}
> +
> +static size_t csum_and_copy_to_pipe_iter(const void *addr, size_t bytes,
> +                               __wsum *csum, struct iov_iter *i)
> +{
> +       struct pipe_inode_info *pipe = i->pipe;
> +       size_t n, r;
> +       size_t off = 0;
> +       __wsum sum = *csum;
> +       int idx;
> +
> +       if (!sanity(i))
> +               return 0;
> +
> +       bytes = n = push_pipe(i, bytes, &idx, &r);
> +       if (unlikely(!n))
> +               return 0;
> +       for ( ; n; idx = next_idx(idx, pipe), r = 0) {
> +               size_t chunk = min_t(size_t, n, PAGE_SIZE - r);
> +               char *p = kmap_atomic(pipe->bufs[idx].page);
> +               sum = csum_and_memcpy(p + r, addr, chunk, sum, off);
> +               kunmap_atomic(p);
> +               i->idx = idx;
> +               i->iov_offset = r + chunk;
> +               n -= chunk;
> +               off += chunk;
> +               addr += chunk;
> +       }
> +       i->count -= bytes;
> +       *csum = sum;
> +       return bytes;
> +}
> +
>  size_t _copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
>  {
>         const char *from = addr;
> @@ -1368,17 +1406,15 @@ size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum,
>                 err ? v.iov_len : 0;
>         }), ({
>                 char *p = kmap_atomic(v.bv_page);
> -               next = csum_partial_copy_nocheck(p + v.bv_offset,
> -                                                (to += v.bv_len) - v.bv_len,
> -                                                v.bv_len, 0);
> +               sum = csum_and_memcpy((to += v.bv_len) - v.bv_len,
> +                                     p + v.bv_offset, v.bv_len,
> +                                     sum, off);
>                 kunmap_atomic(p);
> -               sum = csum_block_add(sum, next, off);
>                 off += v.bv_len;
>         }),({
> -               next = csum_partial_copy_nocheck(v.iov_base,
> -                                                (to += v.iov_len) - v.iov_len,
> -                                                v.iov_len, 0);
> -               sum = csum_block_add(sum, next, off);
> +               sum = csum_and_memcpy((to += v.iov_len) - v.iov_len,
> +                                     v.iov_base, v.iov_len,
> +                                     sum, off);
>                 off += v.iov_len;
>         })
>         )
> @@ -1412,17 +1448,15 @@ bool csum_and_copy_from_iter_full(void *addr, size_t bytes, __wsum *csum,
>                 0;
>         }), ({
>                 char *p = kmap_atomic(v.bv_page);
> -               next = csum_partial_copy_nocheck(p + v.bv_offset,
> -                                                (to += v.bv_len) - v.bv_len,
> -                                                v.bv_len, 0);
> +               sum = csum_and_memcpy((to += v.bv_len) - v.bv_len,
> +                                     p + v.bv_offset, v.bv_len,
> +                                     sum, off);
>                 kunmap_atomic(p);
> -               sum = csum_block_add(sum, next, off);
>                 off += v.bv_len;
>         }),({
> -               next = csum_partial_copy_nocheck(v.iov_base,
> -                                                (to += v.iov_len) - v.iov_len,
> -                                                v.iov_len, 0);
> -               sum = csum_block_add(sum, next, off);
> +               sum = csum_and_memcpy((to += v.iov_len) - v.iov_len,
> +                                     v.iov_base, v.iov_len,
> +                                     sum, off);
>                 off += v.iov_len;
>         })
>         )
> @@ -1438,8 +1472,12 @@ size_t csum_and_copy_to_iter(const void *addr, size_t bytes, __wsum *csum,
>         const char *from = addr;
>         __wsum sum, next;
>         size_t off = 0;
> +
> +       if (unlikely(iov_iter_is_pipe(i)))
> +               return csum_and_copy_to_pipe_iter(addr, bytes, csum, i);
> +
>         sum = *csum;
> -       if (unlikely(iov_iter_is_pipe(i) || iov_iter_is_discard(i))) {
> +       if (unlikely(iov_iter_is_discard(i))) {
>                 WARN_ON(1);     /* for now */
>                 return 0;
>         }
> @@ -1455,17 +1493,15 @@ size_t csum_and_copy_to_iter(const void *addr, size_t bytes, __wsum *csum,
>                 err ? v.iov_len : 0;
>         }), ({
>                 char *p = kmap_atomic(v.bv_page);
> -               next = csum_partial_copy_nocheck((from += v.bv_len) - v.bv_len,
> -                                                p + v.bv_offset,
> -                                                v.bv_len, 0);
> +               sum = csum_and_memcpy(p + v.bv_offset,
> +                                     (from += v.bv_len) - v.bv_len,
> +                                     v.bv_len, sum, off);
>                 kunmap_atomic(p);
> -               sum = csum_block_add(sum, next, off);
>                 off += v.bv_len;
>         }),({
> -               next = csum_partial_copy_nocheck((from += v.iov_len) - v.iov_len,
> -                                                v.iov_base,
> -                                                v.iov_len, 0);
> -               sum = csum_block_add(sum, next, off);
> +               sum = csum_and_memcpy(v.iov_base,
> +                                    (from += v.iov_len) - v.iov_len,
> +                                    v.iov_len, sum, off);
>                 off += v.iov_len;
>         })
>         )