Re: [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes

[Alexander Duyck <alexander.duyck@gmail.com>]

On Tue, Feb 2, 2021 at 11:45 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> When iteratively computing a checksum with csum_block_add, track the
> offset to correctly rotate in csum_block_add when offset is odd.
>
> The open coded implementation of skb_copy_and_csum_datagram did this.
> With the switch to __skb_datagram_iter calling csum_and_copy_to_iter,
> pos was reinitialized to 0 on each call.
>
> Bring back the pos by passing it along with the csum to the callback.
>
> Link: https://lore.kernel.org/netdev/20210128152353.GB27281@optiplex/
> Fixes: 950fcaecd5cc ("datagram: consolidate datagram copy to iter helpers")
> Reported-by: Oliver Graute <oliver.graute@gmail.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> ---
>
> Once the fix makes it to net-next, I'll follow-up with a regression
> test to tools/testing/selftests/net
> ---
>  include/linux/uio.h |  8 +++++++-
>  lib/iov_iter.c      | 24 ++++++++++++++----------
>  net/core/datagram.c |  4 +++-
>  3 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/uio.h b/include/linux/uio.h
> index 72d88566694e..308194b08ca8 100644
> --- a/include/linux/uio.h
> +++ b/include/linux/uio.h
> @@ -260,7 +260,13 @@ static inline void iov_iter_reexpand(struct iov_iter *i, size_t count)
>  {
>         i->count = count;
>  }
> -size_t csum_and_copy_to_iter(const void *addr, size_t bytes, void *csump, struct iov_iter *i);
> +
> +struct csum_state {
> +       __wsum *csump;
> +       size_t off;
> +};
> +

So now that we have a struct maintaining the state I am not sure it
makes sense to be storing a pointer here. You can likely just get away
with storing the checksum value itself and save yourself the extra
pointer chases every time you want to read or write the checksum.

Then it is just the task for the caller to initialize the checksum and
offset, and to copy the checksum to the appropriate pointer when done.
As it stands I am not sure there is much value updating the checksum
when the entire operation could fail anyway and return -EFAULT so it
is probably better to hold off on updating the checksum until you have
computed the entire thing.

> +size_t csum_and_copy_to_iter(const void *addr, size_t bytes, void *csstate, struct iov_iter *i);
>  size_t csum_and_copy_from_iter(void *addr, size_t bytes, __wsum *csum, struct iov_iter *i);
>  bool csum_and_copy_from_iter_full(void *addr, size_t bytes, __wsum *csum, struct iov_iter *i);
>  size_t hash_and_copy_to_iter(const void *addr, size_t bytes, void *hashp,
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index a21e6a5792c5..087235d60514 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -592,14 +592,15 @@ static __wsum csum_and_memcpy(void *to, const void *from, size_t len,
>  }
>
>  static size_t csum_and_copy_to_pipe_iter(const void *addr, size_t bytes,
> -                               __wsum *csum, struct iov_iter *i)
> +                                        struct csum_state *csstate,
> +                                        struct iov_iter *i)
>  {
>         struct pipe_inode_info *pipe = i->pipe;
>         unsigned int p_mask = pipe->ring_size - 1;
> +       __wsum sum = *csstate->csump;
> +       size_t off = csstate->off;
>         unsigned int i_head;
>         size_t n, r;
> -       size_t off = 0;
> -       __wsum sum = *csum;
>
>         if (!sanity(i))
>                 return 0;
> @@ -621,7 +622,8 @@ static size_t csum_and_copy_to_pipe_iter(const void *addr, size_t bytes,
>                 i_head++;
>         } while (n);
>         i->count -= bytes;
> -       *csum = sum;
> +       *csstate->csump = sum;
> +       csstate->off = off;
>         return bytes;
>  }
>
> @@ -1522,18 +1524,19 @@ bool csum_and_copy_from_iter_full(void *addr, size_t bytes, __wsum *csum,
>  }
>  EXPORT_SYMBOL(csum_and_copy_from_iter_full);
>
> -size_t csum_and_copy_to_iter(const void *addr, size_t bytes, void *csump,
> +size_t csum_and_copy_to_iter(const void *addr, size_t bytes, void *_csstate,
>                              struct iov_iter *i)
>  {
> +       struct csum_state *csstate = _csstate;
>         const char *from = addr;
> -       __wsum *csum = csump;
>         __wsum sum, next;
> -       size_t off = 0;
> +       size_t off;
>
>         if (unlikely(iov_iter_is_pipe(i)))
> -               return csum_and_copy_to_pipe_iter(addr, bytes, csum, i);
> +               return csum_and_copy_to_pipe_iter(addr, bytes, _csstate, i);
>
> -       sum = *csum;
> +       sum = *csstate->csump;
> +       off = csstate->off;
>         if (unlikely(iov_iter_is_discard(i))) {
>                 WARN_ON(1);     /* for now */
>                 return 0;
> @@ -1561,7 +1564,8 @@ size_t csum_and_copy_to_iter(const void *addr, size_t bytes, void *csump,
>                 off += v.iov_len;
>         })
>         )
> -       *csum = sum;
> +       *csstate->csump = sum;
> +       csstate->off = off;
>         return bytes;
>  }
>  EXPORT_SYMBOL(csum_and_copy_to_iter);
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 81809fa735a7..c6ac5413dda9 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -721,8 +721,10 @@ static int skb_copy_and_csum_datagram(const struct sk_buff *skb, int offset,
>                                       struct iov_iter *to, int len,
>                                       __wsum *csump)
>  {
> +       struct csum_state csdata = { .csump = csump };
> +
>         return __skb_datagram_iter(skb, offset, to, len, true,
> -                       csum_and_copy_to_iter, csump);
> +                       csum_and_copy_to_iter, &csdata);
>  }
>
>  /**

The rest of this looks good to me, and my only complaint is the
performance nit called out above.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>