Re: [PATCH] net: fix checksum with unaligned buffer

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Jun 21, 2022 at 10:05:15AM +0200, Morten Brørup wrote:
> +TO: @Bruce and @Stephen: You signed off on the 16 bit alignment requirement. We need background info on this.
> 
> > From: Emil Berg [mailto:emil.berg@ericsson.com]
> > Sent: Tuesday, 21 June 2022 09.17
> > 
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: den 20 juni 2022 12:58
> > >
> > > > From: Emil Berg [mailto:emil.berg@ericsson.com]
> > > > Sent: Monday, 20 June 2022 12.38
> > > >
> > > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > Sent: den 17 juni 2022 11:07
> > > > >
> > > > > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > > > Sent: Friday, 17 June 2022 10.45
> > > > > >
> > > > > > With this patch, the checksum can be calculated on an unligned
> > > > > > part
> > > > of
> > > > > > a packet buffer.
> > > > > > I.e. the buf parameter is no longer required to be 16 bit
> > aligned.
> > > > > >
> > > > > > The DPDK invariant that packet buffers must be 16 bit aligned
> > > > remains
> > > > > > unchanged.
> > > > > > This invariant also defines how to calculate the 16 bit
> > checksum
> > > > > > on
> > > > an
> > > > > > unaligned part of a packet buffer.
> > > > > >
> > > > > > Bugzilla ID: 1035
> > > > > > Cc: stable@dpdk.org
> > > > > >
> > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > ---
> > > > > >  lib/net/rte_ip.h | 17 +++++++++++++++--
> > > > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > b502481670..8e301d9c26 100644
> > > > > > --- a/lib/net/rte_ip.h
> > > > > > +++ b/lib/net/rte_ip.h
> > > > > > @@ -162,9 +162,22 @@ __rte_raw_cksum(const void *buf, size_t
> > len,
> > > > > > uint32_t sum)  {
> > > > > >  	/* extend strict-aliasing rules */
> > > > > >  	typedef uint16_t __attribute__((__may_alias__)) u16_p;
> > > > > > -	const u16_p *u16_buf = (const u16_p *)buf;
> > > > > > -	const u16_p *end = u16_buf + len / sizeof(*u16_buf);
> > > > > > +	const u16_p *u16_buf;
> > > > > > +	const u16_p *end;
> > > > > > +
> > > > > > +	/* if buffer is unaligned, keeping it byte order
> > independent */
> > > > > > +	if (unlikely((uintptr_t)buf & 1)) {
> > > > > > +		uint16_t first = 0;
> > > > > > +		if (unlikely(len == 0))
> > > > > > +			return 0;
> > > > > > +		((unsigned char *)&first)[1] = *(const unsigned
> > > > > char *)buf;
> > > > > > +		sum += first;
> > > > > > +		buf = (const void *)((uintptr_t)buf + 1);
> > > > > > +		len--;
> > > > > > +	}
> > > > > >
> > > > > > +	u16_buf = (const u16_p *)buf;
> > > > > > +	end = u16_buf + len / sizeof(*u16_buf);
> > > > > >  	for (; u16_buf != end; ++u16_buf)
> > > > > >  		sum += *u16_buf;
> > > > > >
> > > > > > --
> > > > > > 2.17.1
> > > > >
> > > > > @Emil, can you please test this patch with an unaligned buffer on
> > > > your
> > > > > application to confirm that it produces the expected result.
> > > >
> > > > Hi!
> > > >
> > > > I tested the patch. It doesn't seem to produce the same results. I
> > > > think the problem is that it always starts summing from an even
> > > > address, the sum should always start from the first byte according
> > to
> > > > the checksum specification. Can I instead propose something Mattias
> > > > Rönnblom sent me?
> > >
> > > I assume that it produces the same result when the "buf" parameter is
> > > aligned?
> > >
> > > And when the "buf" parameter is unaligned, I don't expect it to
> > produce the
> > > same results as the simple algorithm!
> > >
> > > This was the whole point of the patch: I expect the overall packet
> > buffer to
> > > be 16 bit aligned, and the checksum to be a partial checksum of such
> > a 16 bit
> > > aligned packet buffer. When calling this function, I assume that the
> > "buf" and
> > > "len" parameters point to a part of such a packet buffer. If these
> > > expectations are correct, the simple algorithm will produce incorrect
> > results
> > > when "buf" is unaligned.
> > >
> > > I was asking you to test if the checksum on the packet is correct
> > when your
> > > application modifies an unaligned part of the packet and uses this
> > function to
> > > update the checksum.
> > >
> > 
> > Now I understand your use case. Your use case seems to be about partial
> > checksums, of which some partial checksums may start on unaligned
> > addresses in an otherwise aligned packet.
> > 
> > Our use case is about calculating the full checksum on a nested packet.
> > That nested packet may start on unaligned addresses.
> > 
> > The difference is basically if we want to sum over aligned addresses or
> > not, handling the heading and trailing bytes appropriately.
> > 
> > Your method does not work in our case since we want to treat the first
> > two bytes as the first word in our case. But I do understand that both
> > methods are useful.
> 
> Yes, that certainly are two different use cases, requiring two different ways of calculating the 16 bit checksum.
> 
> > 
> > Note that your method breaks the API. Previously (assuming no crashing
> > due to low optimization levels, more accepting hardware, or a different
> > compiler (version)) the current method would calculate the checksum
> > assuming the first two bytes is the first word.
> > 
> 
> Depending on the point of view, my patch either fixes a bug (where the checksum was calculated incorrectly when the buf pointer was unaligned) or breaks the API (by calculating the differently when the buffer is unaligned).
> 
> I cannot say with certainty which one is correct, but perhaps some of the people with a deeper DPDK track record can...
> 
> @Bruce and @Stephen, in 2019 you signed off on a patch [1] introducing a 16 bit alignment requirement to the Ethernet address structure.
> 
> It is my understanding that DPDK has an invariant requiring packets to be 16 bit aligned, which that patch supports. Is this invariant documented anywhere, or am I completely wrong? If I'm wrong, then the alignment requirement introduced in that patch needs to be removed, as well as any similar alignment requirements elsewhere in DPDK.

I don't believe it is explicitly documented as a global invariant, but I
think it should be unless there is a definite case where we need to allow
packets to be completely unaligned. Across all packet headers we looked at,
there was no tunneling protocol where the resulting packet was left
unaligned.

That said, if there are real use cases where we need to allow packets to
start at an unaligned address, then I agree with you that we need to roll
back the patch and work to ensure everything works with unaligned
addresses.

/Bruce

> 
> [1] http://git.dpdk.org/dpdk/commit/lib/librte_net/rte_ether.h?id=da5350ef29afd35c1adabe76f60832f3092269ad
> 
> @Emil, we should wait for a conclusion about the alignment invariant before we proceed.
> 
> If there is no such invariant, my patch is wrong, and we need to provide a v2 of the patch, which will then fit your use case.
> If there is such an invariant, my patch is correct, and another function must be added for your use case.
>