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

[Davide Caratti <dcaratti@redhat.com>]

On Mon, 2017-01-23 at 12:59 -0800, Tom Herbert wrote:
> > > > It might make sense to create some CRC helper functions, but last time
> > > > I checked there are so few users of CRC in skbufs I'm not even sure
> > > > that would make sense.

hello Tom and David,

after some (thinking + testing) time, I'm going to re-post this RFC as v2 with
some feedbacks. Thank you in advance for looking at it!

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:
> On Thu, 2017-02-02 at 16:07 +0100, Davide Caratti wrote:
> > This is exactly the cause of issues I see with SCTP. These packets can be
> > wrongly checksummed using skb_checksum_help, or simply not checksummed at
> > all; and in both cases, the packet goes out from the NIC with wrong L4
> > checksum.
> > 
> Okay, makes sense. Please consider doing the following:
> 
> - Add a bit to skbuf called something like "csum_not_inet". When
> ip_summed == CHECKSUM_PARTIAL and this bit is set that means we are
> dealing with something other than an Internet checksum.

Ok, done. Another solution would be to extend possible values of
skb->ip_summed, and define a new value suitable for identifying
not-yet-checksummed SCTP packets (something like CRC32C_PARTIAL). Since
skb->ip_summed is 2-bit wide, the overall effect on skb metadata is the
same as adding skb->csum_not_inet [1].

> - At the top of skb_checksum_help (or maybe before the point where the
> inet specific checksum start begins do something like:
> 
>    if (unlikely(skb->csum_not_inet))
>        return skb_checksum_help_not_inet(...);
> 
>    The rest of skb_checksum_help should remained unchanged.

According to documentation [2], validate_xmit_skb() is a good place where
the if() statement above can be done, to preserve the possibility of having
the CRC32c computation offloaded by the NIC hardware:

if (unlikely(skb->csum_not_inet && !(features & NETIF_F_SCTP_CRC))
	       return skb_checksum_help_not_inet(...);

On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:
> 
> 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!

Ideally, a driver not able to offload checksum computation should call
skb_checksum_help() or skb_sctp_csum_help() to resolve CHECKSUM_PARTIAL
and turn it to CHECKSUM_NONE.
But this wouldn't solve all possible setups: there can be scenarios
where the NIC is configured with NETIF_F_SCTP_CRC set and NETIF_F_CSUM_HW
cleared (it's evil, but possible). In this situation, non-GSO SCTP packets
having CHECKSUM_PARTIAL will be systematically corrupted when they are
processed by validate_xmit_skb().

On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:

> 
> - Add a description of the new bit and how skb_checksum_help can work
> to the comments for CHECKSUM_PARTIAL in skbuff.h

Done.

> 
> - Add FCOE to the list of protocol that can set CHECKSUM_UNNECESSARY
> for a CRC/csum

Done.

> 
> - Add a note to CHECKSUM_COMPLETE section that it can only refer to an
> Internet checksum

Done.

/* references + notes */

[1] ... this recalls to latest comment from David Laight:
On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:
> 
> 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.

According to domumentation, CHECKSUM_COMPLETE and CHECKSUM_UNNECESSARY are
not used for SCTP (nor in the TX path at all); nevertheless, IPVS snat/dnat
actually set CHECKSUM_UNNECESSARY on SCTP packets after the checksum is
updated (see 97203abe6bc4 "net: ipvs: sctp: do not recalc...).

I'm not sure if setting CHECKSUM_UNNECESSARY fits my case, because this would
implicitly skip RX validation when using devices like veth or loopback.

[2] Documentation/networking/checksum_offloads.txt

regards,