On Tue, Feb 2, 2021 at 8:50 PM Alexander Duyck <alexander.du...@gmail.com> wrote: > > On Tue, Feb 2, 2021 at 11:45 AM Willem de Bruijn > <willemdebruijn.ker...@gmail.com> wrote: > > > > From: Willem de Bruijn <will...@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.gra...@gmail.com> > > Signed-off-by: Willem de Bruijn <will...@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 <alexanderdu...@fb.com>
Thanks a lot for reviewing. Good point on the unnecessary pointer. I'll revise that as suggested.
