RE: [RFC PATCH net-next 2/5] net: split skb_checksum_help

[David Laight <David.Laight@ACULAB.COM>]

From: Davide Caratti
> Sent: 02 February 2017 15:07
> > From: Tom Herbert
> > >
> > > Sent: 23 January 2017 21:00
> > ..
> > >
> > > skb_checksum_help is specific to the Internet checksum. For instance,
> > > CHECKSUM_COMPLETE can _only_ refer to Internet checksum calculation
> > > nothing else will work. Checksums and CRCs are very different things
> > > with very different processing. They are not interchangeable, have
> > > very different properties, and hence it is a mistake to try to shoe
> > > horn things so that they use a common infrastructure.
> > >
> 
> true, we don't need to test CHECKSUM_COMPLETE on skbs carrying SCTP.
> So maybe we can simply replace patches 2/5 and 3/5 with the smaller one at
> the bottom of this message.

I have to admit to not knowing exactly what the CHECKSUM_xxx flags actually mean.
I have a good idea about what the intention is though.

...
> On Tue, 2017-01-24 at 16:35 +0000, David Laight wrote:
> >
> > I can imagine horrid things happening if someone tries to encapsulate
> > SCTP/IP in UDP (or worse UDP/IP in SCTP).
> >
> > For UDP in UDP I suspect that CHECKSUM_COMPLETE on an inner UDP packet
> > allows the outer checksum be calculated by ignoring the inner packet
> > (since it sums to zero).
> > This just isn't true if SCTP is involved.
> > There are tricks to generate a crc of a longer packet, but they'd only
> > work for SCTP in SCTP.
> >
> > For non-encapsulated packets it is a different matter.
> 
> If we limit the scope to skbs having ip_summed equal to CHECKSUM_PARTIAL,
> like it's done in patch 4, we only need checksumming the packet starting
> from csum_start to its end, and copy the computed value to csum_offset.
> The difficult thing is discriminating skbs that need CRC32c, namely SCTP,
> from the rest of the traffic (that will likely be checksummed by
> skb_checksum_help).
...

I'm guessing that the SCTP code only sets CHECKSUM_PARTIAL (and doesn't
perform the checksum) if it somehow knows that the target interface
supports CRC32c checksums.

I'd put the onus on any such interface to perform the checksum (and
set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the 
message onto an interface that doesn't advertise CRC32 support.

You certainly don't want to have to go through all the ethernet drivers!

> 
> ------------------- 8< --------------------------
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -200,7 +200,8 @@
>  *accordingly. Note the there is no indication in the skbuff that the
>  *CHECKSUM_PARTIAL refers to an FCOE checksum, a driver that supports
>  *both IP checksum offload and FCOE CRC offload must verify which offload
> - *is configured for a packet presumably by inspecting packet headers.
> + *is configured for a packet presumably by inspecting packet headers; in
> + *case, skb_sctp_csum_help is provided to compute CRC on SCTP packets.
>  *
>  * E. Checksumming on output with GSO.
>  *
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ad5959e..fa9be6d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2580,6 +2580,42 @@ int skb_checksum_help(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(skb_checksum_help);
> 
> +int skb_sctp_csum_help(struct sk_buff *skb)
> +{
> +	__le32 crc32c_csum;
> +	int ret = 0, offset;
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		goto out;
> +	if (unlikely(skb_is_gso(skb)))
> +		goto out;
> +	if (skb_has_shared_frag(skb)) {
> +		ret = __skb_linearize(skb);

I don't think you really want to linearize the packet.
...

	David