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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Tue, Oct 19, 2021 at 7:46 AM Cyril Strejc <cyril.strejc@skoda.cz> 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.

> 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 <cyril.strejc@skoda.cz>
> ---
>  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));

>                 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
>