* [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes
@ 2021-02-02 19:45 Willem de Bruijn
2021-02-03 1:50 ` Alexander Duyck
0 siblings, 1 reply; 3+ messages in thread
From: Willem de Bruijn @ 2021-02-02 19:45 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, oliver.graute, sagi, viro, hch, alexander.duyck,
eric.dumazet, Willem de Bruijn
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;
+};
+
+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);
}
/**
--
2.30.0.365.g02bc693789-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes
2021-02-02 19:45 [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes Willem de Bruijn
@ 2021-02-03 1:50 ` Alexander Duyck
2021-02-03 3:10 ` Willem de Bruijn
0 siblings, 1 reply; 3+ messages in thread
From: Alexander Duyck @ 2021-02-03 1:50 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Netdev, David Miller, Jakub Kicinski, oliver.graute,
Sagi Grimberg, Alexander Viro, Christoph Hellwig, Eric Dumazet,
Willem de Bruijn
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes
2021-02-03 1:50 ` Alexander Duyck
@ 2021-02-03 3:10 ` Willem de Bruijn
0 siblings, 0 replies; 3+ messages in thread
From: Willem de Bruijn @ 2021-02-03 3:10 UTC (permalink / raw)
To: Alexander Duyck
Cc: Willem de Bruijn, Netdev, David Miller, Jakub Kicinski,
oliver.graute, Sagi Grimberg, Alexander Viro, Christoph Hellwig,
Eric Dumazet
On Tue, Feb 2, 2021 at 8:50 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> 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>
Thanks a lot for reviewing. Good point on the unnecessary pointer.
I'll revise that as suggested.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-03 3:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-02 19:45 [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes Willem de Bruijn
2021-02-03 1:50 ` Alexander Duyck
2021-02-03 3:10 ` Willem de Bruijn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).