Re: [PATCH] net: multicast: calculate csum of looped-back and forwarded packets
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2021-10-22 19:08:46
On Tue, Oct 19, 2021 at 7:46 AM Cyril Strejc [off-list ref] wrote:
During a testing of an user-space application which transmits UDP
multicast datagrams and utilizes multicast routing to send the UDP
datagrams out of defined network interfaces, I've found a multicast
router does not fill-in UDP checksum into locally produced, looped-back
and forwarded UDP datagrams, if an original output NIC the datagrams
are sent to has UDP TX checksum offload enabled.
The datagrams are sent malformed out of the NIC the datagrams have been
forwarded to.
It is because:
1. If TX checksum offload is enabled on an output NIC, UDP checksum
is not calculated by kernel and is not filled into skb data.
2. dev_loopback_xmit(), which is called solely by
ip_mc_finish_output(), sets skb->ip_summed = CHECKSUM_UNNECESSARY
unconditionally.
3. Since 35fc92a9 ("[NET]: Allow forwarding of ip_summed except
CHECKSUM_COMPLETE"), the ip_summed value is preserved during
forwarding.
4. If ip_summed != CHECKSUM_PARTIAL, checksum is not calculated during
a packet egress.
We could fix this as follows:
1. Not set CHECKSUM_UNNECESSARY in dev_loopback_xmit(), because it
is just not true.I think this is the right approach. The receive path has to be able to handle packets looped from the transmit path with CHECKSUM_PARTIAL set.
quoted hunk ↗ jump to hunk
2. I assume, the original idea behind setting CHECKSUM_UNNECESSARY in dev_loopback_xmit() is to prevent checksum validation of looped-back local multicast packets. We can adjust __skb_checksum_validate_needed() to handle this as the special case. Signed-off-by: Cyril Strejc <redacted> --- include/linux/skbuff.h | 4 +++- net/core/dev.c | 1 - 2 files changed, 3 insertions(+), 2 deletions(-)diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 841e2f0f5240..95aa0014c3d6 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h@@ -4048,7 +4048,9 @@ static inline bool __skb_checksum_validate_needed(struct sk_buff *skb, bool zero_okay, __sum16 check) { - if (skb_csum_unnecessary(skb) || (zero_okay && !check)) { + if (skb_csum_unnecessary(skb) || + (zero_okay && !check) || + skb->pkt_type == PACKET_LOOPBACK) {
This should not be needed, as skb_csum_unnecessary already handles
CHECKSUM_PARTIAL?
return ((skb->ip_summed == CHECKSUM_UNNECESSARY) ||
skb->csum_valid ||
(skb->ip_summed == CHECKSUM_PARTIAL &&
skb_checksum_start_offset(skb) >= 0));
quoted hunk ↗ jump to hunk
skb->csum_valid = 1; __skb_decr_checksum_unnecessary(skb); return false;diff --git a/net/core/dev.c b/net/core/dev.c index 7ee9fecd3aff..ba4a0994d97b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -3906,7 +3906,6 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb) skb_reset_mac_header(skb); __skb_pull(skb, skb_network_offset(skb)); skb->pkt_type = PACKET_LOOPBACK; - skb->ip_summed = CHECKSUM_UNNECESSARY; WARN_ON(!skb_dst(skb)); skb_dst_force(skb); netif_rx_ni(skb); --2.25.1