Re: [PATCH] net: multicast: calculate csum of looped-back and forwarded packets

On 10/22/21 9:08 PM, Willem de Bruijn wrote:

quoted

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.

As You clarified, the receive path handles CHECKSUM_PARTIAL.

There is a problem with CHECKSUM_NONE -- the case when TX checksum
offload is not supported by a NIC. Kernel does not set
CHECKSUM_UNNECESSARY with a correct value of csum_level when a packet
is being prepared for transmission, but just set the CHECKSUM_NONE.

quoted

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?

Still we need some solution for the CHECKSUM_NONE case which triggers
checksum validation.

quoted

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

Alternatively, we could solve the CHECKSUM_NONE case by a simple,
practical and historical compatible "TX->RX translation" of ip_summed
in dev_loopback_xmit(), which keeps CHECKSUM_PARTIAL and leaves
__skb_checksum_validate_needed() as is:

	if (skb->ip_summed == CHECKSUM_NONE)
		skb->ip_summed = CHECKSUM_UNNECESSARY;

or:
	if (skb->ip_summed != CHECKSUM_PARTIAL)
		skb->ip_summed = CHECKSUM_UNNECESSARY;