Re: IPv6-UDP 0x0000 checksum

[Johannes Berg]

On Thu, 2017-01-26 at 07:27 -0800, Eric Dumazet wrote:
> 
> if (!uh->check && !udp_sk(sk)->no_check6_rx) {
> udp6_csum_zero_error(skb);
> goto csum_error;
> }



Yeah, I'd found no_check6_rx already, was still trying to figure out this one :)

Looks like we actually check uh->check regardless of anything the
driver said (CHECKSUM_UNNECESSARY, or whatever), so we should be fine
even with the hardware tagging it as good in this case.

johannes

Re: IPv6-UDP 0x0000 checksum

[Johannes Berg]

On Thu, 2017-01-26 at 07:24 -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 15:49 +0100, Johannes Berg wrote:
> 
> > Unfortunately, I haven't been able to actually test this yet. I
> > also
> > didn't find the code that would drop frames with CSUM 0 either, so
> > I'm
> > thinking - for now - that if all the csum handling is skipped,
> > dropping
> > 0 csum frames would also be, and then we'd accept a frame we should
> > actually have dropped.
> > 
> > I'll go test this I guess :)
> > 
> > Any pointers to where 0 csum frames are dropped?
> 
> Probably in udp6_csum_init()

Well, now that I see that, I see that they're actually valid in some
circumstances. Oops. :)

Will need to revisit, and check how we set no_check6_rx, etc.

johannes

Re: IPv6-UDP 0x0000 checksum

[Eric Dumazet]

On Thu, 2017-01-26 at 07:24 -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 15:49 +0100, Johannes Berg wrote:
> 
> > Unfortunately, I haven't been able to actually test this yet. I also
> > didn't find the code that would drop frames with CSUM 0 either, so I'm
> > thinking - for now - that if all the csum handling is skipped, dropping
> > 0 csum frames would also be, and then we'd accept a frame we should
> > actually have dropped.
> > 
> > I'll go test this I guess :)
> > 
> > Any pointers to where 0 csum frames are dropped?
> 
> Probably in udp6_csum_init()

vi +804 net/ipv6/udp.c

if (!uh->check && !udp_sk(sk)->no_check6_rx) {
udp6_csum_zero_error(skb);
goto csum_error;
}

Re: IPv6-UDP 0x0000 checksum

[Eric Dumazet]

On Thu, 2017-01-26 at 15:49 +0100, Johannes Berg wrote:

> Unfortunately, I haven't been able to actually test this yet. I also
> didn't find the code that would drop frames with CSUM 0 either, so I'm
> thinking - for now - that if all the csum handling is skipped, dropping
> 0 csum frames would also be, and then we'd accept a frame we should
> actually have dropped.
> 
> I'll go test this I guess :)
> 
> Any pointers to where 0 csum frames are dropped?

Probably in udp6_csum_init()

Re: IPv6-UDP 0x0000 checksum

[Johannes Berg]

On Thu, 2017-01-26 at 06:45 -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 14:49 +0100, Johannes Berg wrote:
> 
> > Oops, sorry - receive. We can only indicate "CHECKSUM_UNNECESSARY",
> > nothing more advanced right now, but right now we'd indicate that
> > if
> > the packet had 0x in the checksum field, but should've had
> > 0x.
> > 
> > On TX I believe we actually do in HW exactly what your patch just
> > did.
> 
> Can you describe the visible effects of this problem ?
> 
> Is that because of a conversion we might do later to
> CHECKSUM_COMPLETE ?

Unfortunately, I haven't been able to actually test this yet. I also
didn't find the code that would drop frames with CSUM 0 either, so I'm
thinking - for now - that if all the csum handling is skipped, dropping
0 csum frames would also be, and then we'd accept a frame we should
actually have dropped.

I'll go test this I guess :)

Any pointers to where 0 csum frames are dropped?

johannes

Re: IPv6-UDP 0x0000 checksum

[Eric Dumazet]

On Thu, 2017-01-26 at 14:49 +0100, Johannes Berg wrote:

> Oops, sorry - receive. We can only indicate "CHECKSUM_UNNECESSARY",
> nothing more advanced right now, but right now we'd indicate that if
> the packet had 0x in the checksum field, but should've had 0x.
> 
> On TX I believe we actually do in HW exactly what your patch just did.

Can you describe the visible effects of this problem ?

Is that because of a conversion we might do later to CHECKSUM_COMPLETE ?

Re: IPv6-UDP 0x0000 checksum

[Johannes Berg]

On Thu, 2017-01-26 at 05:44 -0800, Eric Dumazet wrote:
> On Thu, 2017-01-26 at 14:27 +0100, Johannes Berg wrote:
> > Hi,
> > 
> > It looks like right now we may have a hardware bug and accept
> > 0x as
> > valid, when the outcome of the calculation is 0x.
> > 
> > What do you think we should do about this?
> > 
> > We could ignore the issue entirely, since 0 wasn't ever supposed to
> > be
> > sent anyway - but then we don't drop frames that we should drop. I
> > didn't manage to find the code in the IPv6/UDP stack that even does
> > that, but I assume it's there somewhere.
> > 
> > Alternatively, we could parse the packet to find the checksum
> > inside,
> > and if it's 0 then don't report CHECKSUM_UNNECESSARY, but that
> > seems
> > rather expensive/difficult due to the IPv6 variable header and all
> > that. If we wanted to go this route, are there any helper functions
> > for
> > this?
> > 
> > Unfortunately, in the current devices, we neither have a complete
> > indication that the packet was even UDP-IPv6, nor do we have the
> > raw
> > csum or anything like that. I think they're adding that to the next
> > hardware spin, but we probably need to address this issue now.

> Is this a xmit or rcv problem ?

Oops, sorry - receive. We can only indicate "CHECKSUM_UNNECESSARY",
nothing more advanced right now, but right now we'd indicate that if
the packet had 0x in the checksum field, but should've had 0x.

On TX I believe we actually do in HW exactly what your patch just did.

johannes

Re: IPv6-UDP 0x0000 checksum

[Eric Dumazet]

On Thu, 2017-01-26 at 14:27 +0100, Johannes Berg wrote:
> Hi,
> 
> It looks like right now we may have a hardware bug and accept 0x as
> valid, when the outcome of the calculation is 0x.
> 
> What do you think we should do about this?
> 
> We could ignore the issue entirely, since 0 wasn't ever supposed to be
> sent anyway - but then we don't drop frames that we should drop. I
> didn't manage to find the code in the IPv6/UDP stack that even does
> that, but I assume it's there somewhere.
> 
> Alternatively, we could parse the packet to find the checksum inside,
> and if it's 0 then don't report CHECKSUM_UNNECESSARY, but that seems
> rather expensive/difficult due to the IPv6 variable header and all
> that. If we wanted to go this route, are there any helper functions for
> this?
> 
> Unfortunately, in the current devices, we neither have a complete
> indication that the packet was even UDP-IPv6, nor do we have the raw
> csum or anything like that. I think they're adding that to the next
> hardware spin, but we probably need to address this issue now.
> 
> johannes

Hi Johannes

I am afraid information is missing.

Is this a xmit or rcv problem ?

I recently fixed an issue, could this be this ?

commit 4f2e4ad56a65f3b7d64c258e373cb71e8d2499f4
Author: Eric Dumazet 
Date:   Sat Oct 29 11:02:36 2016 -0700

net: mangle zero checksum in skb_checksum_help()

Sending zero checksum is ok for TCP, but not for UDP.

UDPv6 receiver should by default drop a frame with a 0 checksum,
and UDPv4 would not verify the checksum and might accept a corrupted
packet.

Simply replace such checksum by 0x, regardless of transport.

This error was caught on SIT tunnels, but seems generic.

Signed-off-by: Eric Dumazet 
Cc: Maciej Żenczykowski 
Cc: Willem de Bruijn 
Acked-by: Maciej Żenczykowski 
Signed-off-by: David S. Miller 

diff --git a/net/core/dev.c b/net/core/dev.c
index 
820bac239738eb021354ac95ca5bbdff1840cb8e..eaad4c28069ff523ac784bf2dffd0acff82341a0
 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2484,7 +2484,7 @@ int skb_checksum_help(struct sk_buff *skb)
goto out;
}
 
-   *(__sum16 *)(skb->data + offset) = csum_fold(csum);
+   *(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
 out_set_summed:
skb->ip_summed = CHECKSUM_NONE;
 out:

IPv6-UDP 0x0000 checksum

[Johannes Berg]

Hi,

It looks like right now we may have a hardware bug and accept 0x as
valid, when the outcome of the calculation is 0x.

What do you think we should do about this?

We could ignore the issue entirely, since 0 wasn't ever supposed to be
sent anyway - but then we don't drop frames that we should drop. I
didn't manage to find the code in the IPv6/UDP stack that even does
that, but I assume it's there somewhere.

Alternatively, we could parse the packet to find the checksum inside,
and if it's 0 then don't report CHECKSUM_UNNECESSARY, but that seems
rather expensive/difficult due to the IPv6 variable header and all
that. If we wanted to go this route, are there any helper functions for
this?

Unfortunately, in the current devices, we neither have a complete
indication that the packet was even UDP-IPv6, nor do we have the raw
csum or anything like that. I think they're adding that to the next
hardware spin, but we probably need to address this issue now.

johannes