| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+FuTScNiQ2iicjOOumDeYM5kcQDew0DBjxB43JLGwqj+PVsyg@mail.gmail.com>
Date: Tue, 2 Feb 2021 22:10:02 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Netdev <netdev@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, oliver.graute@...il.com,
Sagi Grimberg <sagi@...mberg.me>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christoph Hellwig <hch@....de>,
Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net] udp: fix skb_copy_and_csum_datagram with odd segment sizes
On Tue, Feb 2, 2021 at 8:50 PM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Tue, Feb 2, 2021 at 11:45 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > From: Willem de Bruijn <willemb@...gle.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@...il.com>
> > Signed-off-by: Willem de Bruijn <willemb@...gle.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@...com>
Thanks a lot for reviewing. Good point on the unnecessary pointer.
I'll revise that as suggested.
Powered by blists - more mailing lists