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

On Sat, Oct 23, 2021 at 7:26 PM Cyril Strejc [off-list ref] wrote:

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

quoted

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

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

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;

Based on the idea that these packets are fully checksummed, so even if
they loop to the tx path again with ip_summed CHECKSUM_UNNECESSARY,
they will not cause the bug that you originally reported?

Yes, that looks like a nice solution.

I wonder what the behavior is for unicast packets. As it makes sense
for the two to work the same. For instance, packets traveling over
veth_xmit to a local socket. Their ip_summed is not adjusted as far as
I know. If created with CHECKSUM_NONE, these will then incur an
unnecessary checksum validation, too. Such packets are built, e.g.,
for udp sockets with corking. They could benefit from a similar
solution. Not suggesting for this patch, to be clear.