| Message ID | 1485438299.5145.117.camel@edumazet-glaptop3.roam.corp.google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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 > > 0x0000 as > > valid, when the outcome of the calculation is 0xffff. > > > > 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 0x0000 in the checksum field, but should've had 0xffff. On TX I believe we actually do in HW exactly what your patch just did. johannes
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 0x0000 in the checksum field, but should've had 0xffff. > > 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 ?
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 0x0000 in the checksum field, but should've had > > 0xffff. > > > > 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
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()
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; }
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
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
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: