Skip to content

fix CRC32c in the forwarding path

Davide Caratti linux-sctp netdev 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest7 patches 1 replyIn review
  2. v27 patches 1 replySuperseded
  3. v17 patches 1 replySuperseded

Changes in v3 from the cover letter

- patch 1/7: more standard declaration of stub variables

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/7skbuff: add stub to help computing crc32c on SCTP packetsinclude/linux/skbuff.h net/core/skbuff.c net/sctp/offload.c+34 -00 replies
2/7net: introduce skb_crc32c_csum_helpinclude/linux/netdevice.h include/linux/skbuff.h net/core/dev.c+44 -10 replies
3/7sk_buff: remove support for csum_bad in sk_buffdrivers/net/ethernet/aquantia/atlantic/aq_ring.c include/linux/netdevice.h include/linux/skbuff.h +4 more+6 -360 replies
4/7net: use skb->csum_not_inet to identify packets needing crc32cinclude/linux/skbuff.h net/core/dev.c net/sched/act_csum.c +2 more+13 -70 replies
5/7net: more accurate checksumming in validate_xmit_skb()Documentation/networking/checksum-offloads.txt include/linux/netdevice.h include/linux/skbuff.h +1 more+27 -140 replies
6/7openvswitch: more accurate checksumming in queue_userspace_packet()net/openvswitch/datapath.c+1 -10 replies
7/7sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY}include/linux/skbuff.h+5 -20 replies

Reviews trailers per patch

2 of 7 patches have a Reviewed-by or Acked-by.

PatchTom Herbert
1/7
2/7
3/7
4/7A-b
5/7
6/7
7/7A-b

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v3 thread

Cover letter [PATCH net-next v3 0/7] fix CRC32c in the forwarding path

Current kernel allows offloading CRC32c computation when SCTP packets are generated, setting skb->ip_summed to CHECKSUM_PARTIAL, if the underlying device features have NETIF_F_SCTP_CRC set. However, after these packets are forwarded, they may land on a device where CRC32c offloading is not available: as a consequence, transmission is done with wrong CRC32c. It's not possible to use sctp_compte_cksum() in the forwarding path and in most drivers, because it needs symbols exported by libcrc32c module.

Patch 1 and 2 of this series try to solve this problem, introducing a new helper function, namely skb_crc32c_csum_help(), that can be used to resolve CHECKSUM_PARTIAL when crc32c is needed instead of Internet Checksum.

Currently, we need to parse the packet headers to understand what algorithm is needed to resolve CHECKSUM_PARTIAL. We can speedup things by storing this information in the skb metadata, and use it to call an appropriate helper (skb_checksum_help or skb_crc32c_csum_help), or leave the packet unmodified when the NIC is able to offload the checksum computation.

Patch 3 deprecates skb->csum_bad to free one bit in skb metadata; patch 4 introduces skb->csum_not_inet, providing skb with an indication on the algorithm needed to resolve CHECKSUM_PARTIAL.
Patch 5 and 6 fix the kernel forwarding path and openvswitch datapath, where skb_checksum_help was unconditionally called to resolve CHECKSUM_PARTIAL, thus generating wrong CRC32c in forwarded SCTP packets.
Finally, patch 7 updates documentation to provide a better description of possible values of skb->ip_summed.

Some further work is still possible:
* drivers that parse the packet header to correctly resolve CHECKSUM_PARTIAL (e.g. ixgbe_tx_csum()) can benefit from testing skb->csum_not_inet to avoid calling ip_hdr(skb)->protocol or ixgbe_ipv6_csum_is_sctp(skb).

* drivers that call skb_checksum_help() to resolve CHECKSUM_PARTIAL can call skb_csum_hwoffload_help to avoid corrupting SCTP packets.

Changes v2->v3:
- patch 1/7: more standard declaration of stub variables

Changes v1->v2:
- none

Changes RFCv4->v1:
- patch 2/7: use WARN_ON_ONCE() instead of BUG_ON(), and avoid computing CRC32c on the error path.
- patch 3/7: don't invert tests on the values of same_flow and NAPI_GRO_CB(skb)->flush in dev_gro_receive(), it's useless and it breaks GRO functionality as reported by kernel test robot.

Davide Caratti (7):
skbuff: add stub to help computing crc32c on SCTP packets
net: introduce skb_crc32c_csum_help
sk_buff: remove support for csum_bad in sk_buff
net: use skb->csum_not_inet to identify packets needing crc32c
net: more accurate checksumming in validate_xmit_skb()
openvswitch: more accurate checksumming in queue_userspace_packet() sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY}

 Documentation/networking/checksum-offloads.txt   | 11 +++--
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c |  2 +-
 include/linux/netdevice.h                        |  8 ++--
 include/linux/skbuff.h                           | 58 +++++++++--------------
 net/bridge/netfilter/nft_reject_bridge.c         |  5 +-
 net/core/dev.c                                   | 59 ++++++++++++++++++++++--
 net/core/skbuff.c                                | 26 +++++++++++
 net/ipv4/netfilter/nf_reject_ipv4.c              |  2 +-
 net/ipv6/netfilter/nf_reject_ipv6.c              |  3 --
 net/openvswitch/datapath.c                       |  2 +-
 net/sched/act_csum.c                             |  1 +
 net/sctp/offload.c                               |  7 +++
 net/sctp/output.c                                |  1 +
 13 files changed, 127 insertions(+), 58 deletions(-)
Signature · 1 line
2.7.4