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, -- davide -- To unsubscribe from this list: send the line "unsubscribe linux-sctp" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
