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

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

[Cyril Strejc]

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.

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

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

[Willem de Bruijn]

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
>

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

[Cyril Strejc]

On 10/22/21 9:08 PM, Willem de Bruijn wrote:
>> 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.

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

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

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

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

[Willem de Bruijn]

On Sat, Oct 23, 2021 at 7:26 PM Cyril Strejc <cyril.strejc@skoda.cz> wrote:
>
> On 10/22/21 9:08 PM, Willem de Bruijn wrote:
> >> 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.
>
> >> 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?
> >
>
> Still we need some solution for the CHECKSUM_NONE case which triggers
> checksum validation.
>
> >> 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.

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

[Cyril Strejc]

On 2021-10-23T22:41:06-0400, Willem de Bruijn wrote:
> >
> > 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?
> 

It won't cause the bug. The original bug is caused solely by
CHECKSUM_PARTIAL being unconditionally translated to
CHECKSUM_UNNECESSARY in dev_loopback_xmit(). Adding the condition to
keep CHECKSUM_PARTIAL solves the issue.

> Yes, that looks like a nice solution.

I will double-check and send PATCH v2 to this e-mail thread.

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

[Cyril Strejc]

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

The minimum fix in dev_loopback_xmit():

1. Preserves skb->ip_summed CHECKSUM_PARTIAL. This is the
   case when the original output NIC has TX checksum offload enabled.
   The effects are:

     a) If the forwarding destination interface supports TX checksum
        offloading, the NIC driver is responsible to fill-in the
        checksum.

     b) If the forwarding destination interface does NOT support TX
        checksum offloading, checksums are filled-in by kernel before
        skb is submitted to the NIC driver.

     c) For local delivery, checksum validation is skipped as in the
        case of CHECKSUM_UNNECESSARY, thanks to skb_csum_unnecessary().

2. Translates ip_summed CHECKSUM_NONE to CHECKSUM_UNNECESSARY. It
   means, for CHECKSUM_NONE, the behavior is unmodified and is there
   to skip a looped-back packet local delivery checksum validation.

Signed-off-by: Cyril Strejc <cyril.strejc@skoda.cz>
---
 include/net/udp.h | 5 +++--
 net/core/dev.c    | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 360df454356c..909ecf447e0f 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -494,8 +494,9 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
 	 * CHECKSUM_NONE in __udp_gso_segment. UDP GRO indeed builds partial
 	 * packets in udp_gro_complete_segment. As does UDP GSO, verified by
 	 * udp_send_skb. But when those packets are looped in dev_loopback_xmit
-	 * their ip_summed is set to CHECKSUM_UNNECESSARY. Reset in this
-	 * specific case, where PARTIAL is both correct and required.
+	 * their ip_summed CHECKSUM_NONE is changed to CHECKSUM_UNNECESSARY.
+	 * Reset in this specific case, where PARTIAL is both correct and
+	 * required.
 	 */
 	if (skb->pkt_type == PACKET_LOOPBACK)
 		skb->ip_summed = CHECKSUM_PARTIAL;
diff --git a/net/core/dev.c b/net/core/dev.c
index 7ee9fecd3aff..c0009c3f88a0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3906,7 +3906,8 @@ 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;
+	if (skb->ip_summed == CHECKSUM_NONE)
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
 	WARN_ON(!skb_dst(skb));
 	skb_dst_force(skb);
 	netif_rx_ni(skb);
-- 
2.25.1

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

[Willem de Bruijn]

On Sun, Oct 24, 2021 at 4:17 PM 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 the 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.
>
> The minimum fix in dev_loopback_xmit():
>
> 1. Preserves skb->ip_summed CHECKSUM_PARTIAL. This is the
>    case when the original output NIC has TX checksum offload enabled.
>    The effects are:
>
>      a) If the forwarding destination interface supports TX checksum
>         offloading, the NIC driver is responsible to fill-in the
>         checksum.
>
>      b) If the forwarding destination interface does NOT support TX
>         checksum offloading, checksums are filled-in by kernel before
>         skb is submitted to the NIC driver.
>
>      c) For local delivery, checksum validation is skipped as in the
>         case of CHECKSUM_UNNECESSARY, thanks to skb_csum_unnecessary().
>
> 2. Translates ip_summed CHECKSUM_NONE to CHECKSUM_UNNECESSARY. It
>    means, for CHECKSUM_NONE, the behavior is unmodified and is there
>    to skip a looped-back packet local delivery checksum validation.
>
> Signed-off-by: Cyril Strejc <cyril.strejc@skoda.cz>

Reviewed-by: Willem de Bruijn <willemb@google.com>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sun, 24 Oct 2021 22:14:25 +0200 you 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.
> 
> [...]

Here is the summary with links:
  - [v2] net: multicast: calculate csum of looped-back and forwarded packets
    https://git.kernel.org/netdev/net/c/9122a70a6333

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html