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 <redacted>
---
include/linux/skbuff.h | 4 +++-
net/core/dev.c | 1 -
2 files changed, 3 insertions(+), 2 deletions(-)
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
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(-)
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));
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(-)
@@ -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;
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-10-24 02:42:03
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(-)
@@ -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.
On 2021-10-23T22:41:06-0400, Willem de Bruijn wrote:
quoted
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.
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 <redacted>
---
include/net/udp.h | 5 +++--
net/core/dev.c | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2021-10-25 14:32:05
On Sun, Oct 24, 2021 at 4:17 PM 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 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 <redacted>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller [off-list ref]:
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.
[...]