[PATCH] ipv6: ip6mr: Recalc UDP checksum before forwarding

[PATCH] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Brendan McGrath]

Currently, when forwarding from a Virtual Interface to a Physical
Interface, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
the UDP checksum has not been calculated.

When the packet is then forwarded by a Multicast Router, the checksum
value is left as is and therefore rejected by the receiving
machine(s).

This patch ensures the checksum is recalculated before forwarding.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---

It's a bit ugly putting UDP specific code in this spot - but I'm not
aware of any other protocols that are:
a) multicast;
b) forwarded; and
c) checksummed

 net/ipv6/ip6mr.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 890f9bda..ee4370a 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2077,6 +2077,13 @@ static int ip6mr_forward2(struct net *net, struct mr6_table *mrt,
 	ipv6h = ipv6_hdr(skb);
 	ipv6h->hop_limit--;
 
+	if (ipv6h->nexthdr == NEXTHDR_UDP &&
+					skb->ip_summed != CHECKSUM_PARTIAL) {
+		struct udphdr *uh = udp_hdr(skb);
+		udp6_set_csum(false, skb, &ipv6_hdr(skb)->saddr,
+					&ipv6_hdr(skb)->daddr, ntohs(uh->len));
+	}
+
 	IP6CB(skb)->flags |= IP6SKB_FORWARDED;
 
 	return NF_HOOK(NFPROTO_IPV6, NF_INET_FORWARD,
-- 
2.7.4

Re: [PATCH] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Marcelo Ricardo Leitner]

Hi,

On Wed, Dec 13, 2017 at 10:20:48PM +1100, Brendan McGrath wrote:
> Currently, when forwarding from a Virtual Interface to a Physical
> Interface, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
> the UDP checksum has not been calculated.
> 
> When the packet is then forwarded by a Multicast Router, the checksum
> value is left as is and therefore rejected by the receiving
> machine(s).
> 
> This patch ensures the checksum is recalculated before forwarding.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> 
> It's a bit ugly putting UDP specific code in this spot - but I'm not
> aware of any other protocols that are:
> a) multicast;
> b) forwarded; and
> c) checksummed
> 
>  net/ipv6/ip6mr.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 890f9bda..ee4370a 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -2077,6 +2077,13 @@ static int ip6mr_forward2(struct net *net, struct mr6_table *mrt,
>  	ipv6h = ipv6_hdr(skb);
>  	ipv6h->hop_limit--;
>  
> +	if (ipv6h->nexthdr == NEXTHDR_UDP &&
> +					skb->ip_summed != CHECKSUM_PARTIAL) {
	    ^

This indentation is wrong. The 2nd line should start right after the (
column in the 1st line, like:
+	if (ipv6h->nexthdr == NEXTHDR_UDP &&
+	    skb->ip_summed != CHECKSUM_PARTIAL) {
Adjust with spaces as needed.

Running the patch through scripts/checkpatch.pl before posting will
catch these.

> +		struct udphdr *uh = udp_hdr(skb);
> +		udp6_set_csum(false, skb, &ipv6_hdr(skb)->saddr,
> +					&ipv6_hdr(skb)->daddr, ntohs(uh->len));
                              ^  same deal here.

> +	}
> +
>  	IP6CB(skb)->flags |= IP6SKB_FORWARDED;
>  
>  	return NF_HOOK(NFPROTO_IPV6, NF_INET_FORWARD,
> -- 
> 2.7.4
> 

Re: [PATCH] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Eric Dumazet]

On Wed, 2017-12-13 at 22:20 +1100, Brendan McGrath wrote:
> Currently, when forwarding from a Virtual Interface to a Physical
> Interface, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
> the UDP checksum has not been calculated.
> 

This seems a bug then ?
CHECKSUM_UNNECESSARY means checksum has been validated.
Not that we want it being computed later in the stack.

If we force a checksum here, what guarantee do we have packet was not
corrupted before we do this ?

Re: [PATCH] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Brendan McGrath]

I should clarify that the packet being forwarded originated on the 
Virtual Interface (i.e. it wasn't received on it). When data is received 
on the Virtual Interface (i.e. sent by a virtual host) then ip_summed is 
CHECKSUM_PARTIAL and the checksum is calculated before transmission on 
the wire.

The scenario I was testing (which caused the csum error) was to:
a) send multicast data to virtual devices over the Virtual Interface; 
and then
b) add an MFC to forward traffic from the Virtual Interface to a 
Physical Interface (so one of the machines on the physical network could 
receive it).

The packet was accepted by the Virtual devices (even though tcpdump 
shows an invalid CRC) but rejected by the Physical devices. My 
assumption here was there is some kind of optimisation (for a 
virtualised Linux kernel on the same host) but I didn't find the code to 
verify that assumption.

My tests and findings were:
MR = Multicast Router
VM = Virtual Machine (connected to the MR via a virtual switch [virbr0])
PH = Physical Machine (connected via a physical switch [br0])

The asterisk indicates where packet originated.
The value of ip_summed is being checked by a netfilter hook registered 
to NF_INET_FORWARD.
The CRC value was checked by tcpdump on the receiving interface.
"Packet accepted" indicates that the application received the packet.

Scenario 1:
VM <------* MR --------> PH

VM CRC: Incorrect (packet accepted)
PH CRC: Incorrect (packet rejected)
ip_summed = 1 (CHECKSUM_UNNECESSARY)


Scenario 2:
VM *------> MR --------> PH

MR(br0) CRC: Incorrect (packet accepted)
PH CRC: Correct (packet accepted)
ip_summed = 3 (CHECKSUM_PARTIAL)


Scenario 3:
VM <------- MR *-------> PH

VM CRC: Correct (packet accepted)
PH CRC: Correct (packet accepted)
ip_summed = 1 (CHECKSUM_UNNECESSARY)


On 14/12/17 04:52, Eric Dumazet wrote:
> On Wed, 2017-12-13 at 22:20 +1100, Brendan McGrath wrote:
>> Currently, when forwarding from a Virtual Interface to a Physical
>> Interface, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
>> the UDP checksum has not been calculated.
>>
> This seems a bug then ?
> CHECKSUM_UNNECESSARY means checksum has been validated.
> Not that we want it being computed later in the stack.
>
> If we force a checksum here, what guarantee do we have packet was not
> corrupted before we do this ?
>

[PATCHv2] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Brendan McGrath]

Currently, when forwarding a multicast packet originating from a
Virtual Interface on a Multicast Router to one of its Physical
Interfaces, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
the UDP checksum is not calculated.

The checksum value of the forwarded packet is left as is and
therefore rejected by the receiving machine(s).

This patch ensures the checksum is recalculated before forwarding.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---

Changes since PATCH v1:
 - fixed formatting
 - clarified in git comment that issue is with packet originating on
multicast router
 - check value of pkt_type instead of ip_summed

To gaurentee we don't correct the checksum on a packet that was
corrupted beforehand, I changed the code to check if pkt_type is
PACKET_LOOPBACK. 

I also found that ip_summed is set to CHECKSUM_UNNECESSARY in
dev_loopback_xmit. So every packet that originates and is forwarded by
the same host will have ip_summed set to CHECKSUM_UNNECESSARY.

Note that this scenario appears to be unique to multicasting as
unicast always originates on the interface where the packet will be
sent (thus a unicast packet is never forwarded by the host on which
it originates).



 net/ipv6/ip6mr.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 890f9bda..96f035f 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -2077,6 +2077,14 @@ static int ip6mr_forward2(struct net *net, struct mr6_table *mrt,
 	ipv6h = ipv6_hdr(skb);
 	ipv6h->hop_limit--;
 
+	if (ipv6h->nexthdr == NEXTHDR_UDP &&
+	    skb->pkt_type == PACKET_LOOPBACK) {
+		struct udphdr *uh = udp_hdr(skb);
+
+		udp6_set_csum(false, skb, &ipv6_hdr(skb)->saddr,
+			      &ipv6_hdr(skb)->daddr, ntohs(uh->len));
+	}
+
 	IP6CB(skb)->flags |= IP6SKB_FORWARDED;
 
 	return NF_HOOK(NFPROTO_IPV6, NF_INET_FORWARD,
-- 
2.7.4

Re: [PATCHv2] ipv6: ip6mr: Recalc UDP checksum before forwarding

[David Miller]

From: Brendan McGrath <redmcg@redmandi.dyndns.org>
Date: Thu, 14 Dec 2017 22:37:03 +1100

> Currently, when forwarding a multicast packet originating from a
> Virtual Interface on a Multicast Router to one of its Physical
> Interfaces, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
> the UDP checksum is not calculated.
> 
> The checksum value of the forwarded packet is left as is and
> therefore rejected by the receiving machine(s).
> 
> This patch ensures the checksum is recalculated before forwarding.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>

I still don't like this, UDP can't be the only protocol that goes
over multicast and might therefore have this problem.

Actually, I'm still also having trouble figuring out how this happens
in the first place.

Please draw a specific detailed network diagram, show the exact
configuration of each interface and exactly what driver runs that
interface, and show where the packet comes from, who creates it, and
where these checksum settings are done that lead to this problem.

Like Eric, I'm very suspicious and I think that there is some problem
with whoever builds or modifies this packet, and the code you are
touching has no business "fixing it up"

Thank you.

Re: [PATCHv2] ipv6: ip6mr: Recalc UDP checksum before forwarding

[Brendan McGrath]

The network diagram is very simple. It is just:
VM <------* MR --------> PH

Where:
MR = Multicast Router
VM = Virtual Machine (connected by a Virtual Interface using the 
'virtio_net' driver)
PH = Physical Host (connected by a physical Ethernet connection)
* = The interface where the packet originates (the 'virtio net' interface)

Due to a MFC entry - the packet is forwarded from the virtio interface 
to the physical interface. There is an assumption in this forwarding 
process that the checksum would already be calculated.

But I have found that with 'tx checksum offloading' on - the 
'virtio_net' driver does not appear to generate a checksum at all. The 
assumption here is that the packet will only ever be seen internal to 
the virtio network.

But this scenario sits outside both those assumptions - hence the issue.

This patch looked to address the assumption made in the forwarding 
process - but I now think the issue is with the virtio assumption. Some 
research on the Internet highlighted other issues with the virtio 
assumption. They are:
1. Applications that look at the entire packet and validate checksum 
themselves (such as some DHCP clients); and
2. Tunnels - where the packet is placed inside a tunnel as is and 
delivered elsewhere

And of course this scenario.

This archived libvirt-users post actually gave me a couple of ideas to try:
https://www.redhat.com/archives/libvirt-users/2016-March/msg00034.html

When I disable tx checksum offloading on the virtio interface (via 
'ethtool -K virbr0 tx off') - the checksum is calculated correctly and 
everything works.

I can also get it to work by adding the following ip6filter entry:
ip6tables -t mangle -A POSTROUTING -o virbr0 -d ff00::/8 -j CHECKSUM 
--checksum-fill

So I think this patch can be withdrawn in favour of one of these two 
workarounds.

On 16/12/17 05:27, David Miller wrote:
> From: Brendan McGrath <redmcg@redmandi.dyndns.org>
> Date: Thu, 14 Dec 2017 22:37:03 +1100
>
>> Currently, when forwarding a multicast packet originating from a
>> Virtual Interface on a Multicast Router to one of its Physical
>> Interfaces, ip_summed is set to a value of CHECKSUM_UNNECESSARY and
>> the UDP checksum is not calculated.
>>
>> The checksum value of the forwarded packet is left as is and
>> therefore rejected by the receiving machine(s).
>>
>> This patch ensures the checksum is recalculated before forwarding.
>>
>> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> I still don't like this, UDP can't be the only protocol that goes
> over multicast and might therefore have this problem.
>
> Actually, I'm still also having trouble figuring out how this happens
> in the first place.
>
> Please draw a specific detailed network diagram, show the exact
> configuration of each interface and exactly what driver runs that
> interface, and show where the packet comes from, who creates it, and
> where these checksum settings are done that lead to this problem.
>
> Like Eric, I'm very suspicious and I think that there is some problem
> with whoever builds or modifies this packet, and the code you are
> touching has no business "fixing it up"
>
> Thank you.