| Message ID | 20231105-gemini-largeframe-fix-v2-4-cd3a5aa6c496@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix large frames in the Gemini ethernet driver | expand |
On Sun, Nov 05, 2023 at 09:57:26PM +0100, Linus Walleij wrote: > The Gemini ethernet controller provides hardware checksumming > for frames up to 1514 bytes including ethernet headers but not > FCS. > > If we start sending bigger frames (after first bumping up the MTU > on both interfaces sending and receiveing the frames), truncated > packets start to appear on the target such as in this tcpdump > resulting from ping -s 1474: A bit related: what is gmac_fix_features() supposed to do? I see it unsets GMAC_OFFLOAD_FEATURES when the MTU goes over a certain limit, and that also includes NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM. Is that limit correct, or is it supposed to kick in sooner, to allow validate_xmit_skb() -> skb_csum_hwoffload_help() do the software checksuum for you? I'm not sure whether that was the intention. > > 23:34:17.241983 14:d6:4d:a8:3c:4f (oui Unknown) > bc:ae:c5:6b:a8:3d (oui Unknown), > ethertype IPv4 (0x0800), length 1514: truncated-ip - 2 bytes missing! > (tos 0x0, ttl 64, id 32653, offset 0, flags [DF], proto ICMP (1), length 1502) > OpenWrt.lan > Fecusia: ICMP echo request, id 1672, seq 50, length 1482 > > If we bypass the hardware checksumming and provide a software > fallback, everything starts working fine up to the max TX MTU > of 2047 bytes, for example ping -s2000 192.168.1.2: > > 00:44:29.587598 bc:ae:c5:6b:a8:3d (oui Unknown) > 14:d6:4d:a8:3c:4f (oui Unknown), > ethertype IPv4 (0x0800), length 2042: > (tos 0x0, ttl 64, id 51828, offset 0, flags [none], proto ICMP (1), length 2028) > Fecusia > OpenWrt.lan: ICMP echo reply, id 1683, seq 4, length 2008 > > The bit enabling to bypass hardware checksum (or any of the > "TSS" bits) are undocumented in the hardware reference manual. > The entire hardware checksum unit appears undocumented. The > conclusion that we need to use the "bypass" bit was found by > trial-and-error. > > Since no hardware checksum will happen, we slot in a software > checksum fallback. > > Check for the condition where we need to compute checksum on the > skb with either hardware or software using == CHECKSUM_PARTIAL instead > of != CHECKSUM_NONE which is an incomplete check according to > <linux/skbuff.h>. > > On the D-Link DIR-685 router this fixes a bug on the conduit > interface to the RTL8366RB DSA switch: as the switch needs to add > space for its tag it increases the MTU on the conduit interface > to 1504 and that means that when the router sends packages > of 1500 bytes these get an extra 4 bytes of DSA tag and the > transfer fails because of the erroneous hardware checksumming, > affecting such basic functionality as the LuCI web interface. > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> To be clear, I didn't suggest any of this. I just pointed towards the gemini.c driver as being the problem. Please remove my Suggested-by tag. > Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/net/ethernet/cortina/gemini.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index 576174a862a9..84295c1b87e6 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -1145,6 +1145,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > dma_addr_t mapping; > unsigned short mtu; > void *buffer; > + int ret; > > mtu = ETH_HLEN; > mtu += netdev->mtu; > @@ -1165,7 +1166,19 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > word3 |= mtu; > } > > - if (skb->ip_summed != CHECKSUM_NONE) { > + if (skb->len >= ETH_FRAME_LEN) { > + /* Hardware offloaded checksumming isn't working on frames > + * bigger than 1514 bytes. Perhaps the buffer is only 1518 > + * bytes fitting a normal frame and a checksum? > + * Just use software checksumming and bypass on bigger frames. > + */ > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > + ret = skb_checksum_help(skb); > + if (ret) > + return ret; > + } > + word1 |= TSS_BYPASS_BIT; > + } else if (skb->ip_summed == CHECKSUM_PARTIAL) { > int tcp = 0; > > if (skb->protocol == htons(ETH_P_IP)) { > > -- > 2.34.1 > [ context: tag_rtl4_a.c is a "category 2", aka "Ethertype", tagger ] We say this in Documentation/networking/dsa/dsa.rst: Checksum offload should work with category 1 and 2 taggers when the DSA conduit driver declares NETIF_F_HW_CSUM in vlan_features and looks at csum_start and csum_offset. For those cases, DSA will shift the checksum start and offset by the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the offload hardware already expects that specific tag (perhaps due to matching vendors). DSA user ports inherit those flags from the conduit, and it is up to the driver to correctly fall back to software checksum when the IP header is not where the hardware expects. If that check is ineffective, the packets might go to the network without a proper checksum (the checksum field will have the pseudo IP header sum). Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol, rather than depending on skb->len?! if (skb->protocol == htons(ETH_P_IP)) { word1 |= TSS_IP_CHKSUM_BIT; tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; } else { /* IPv6 */ word1 |= TSS_IPV6_ENABLE_BIT; tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP; } // here word1 |= TSS_BYPASS_BIT; Gemini should never attempt to provide checksums for DSA-tagged packets unless it is able to take skb->csum_start into consideration, otherwise it will get it wrong. This is somewhat independent of the other problem you've found, which seems to be that large non-DSA packets get truncated anyway. But not bypassing TX checksum offload truncates a packet? Hmm, strange.
On Mon, Nov 06, 2023 at 03:26:26PM +0200, Vladimir Oltean wrote: > Gemini should never attempt to provide checksums for DSA-tagged packets > unless it is able to take skb->csum_start into consideration, otherwise > it will get it wrong. Additionally, since Gemini does not put NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM in vlan_features, DSA won't inherit (thus won't have) them. So, validate_xmit_skb() should perform the skb checksum during the xmit on the user port, which is earlier compared to the xmit on the conduit. So, I guess skb->ip_summed should already be CHECKSUM_NONE here? I think the only problem for DSA is the lack of the TSS_BYPASS_BIT. The rest is unrelated.
On Mon, Nov 6, 2023 at 2:26 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Sun, Nov 05, 2023 at 09:57:26PM +0100, Linus Walleij wrote: > > If we start sending bigger frames (after first bumping up the MTU > > on both interfaces sending and receiveing the frames), truncated > > packets start to appear on the target such as in this tcpdump > > resulting from ping -s 1474: > > A bit related: what is gmac_fix_features() supposed to do? I see it > unsets GMAC_OFFLOAD_FEATURES when the MTU goes over a certain limit, > and that also includes NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM. Is that > limit correct, or is it supposed to kick in sooner, to allow > validate_xmit_skb() -> skb_csum_hwoffload_help() do the software > checksuum for you? I'm not sure whether that was the intention. That indeed seems like the intention. But it's a bit suboptimal, because it disables hardware checksum just because the MTU goes over a certain level, and stops using the hardware checksum also for all packets smaller than the MTU :( I'll delete this and make the driver slot in the SW fallback per-packet instead, I think it is fair to assume that most packets will be < MTU and it is really just a question of where the fallback gets called. > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > > To be clear, I didn't suggest any of this. I just pointed towards the gemini.c > driver as being the problem. Please remove my Suggested-by tag. OK sorry, it was just my way of trying to provide credit where credit is due, because you helped so much with this bug. > > - if (skb->ip_summed != CHECKSUM_NONE) { > > + if (skb->len >= ETH_FRAME_LEN) { > > + /* Hardware offloaded checksumming isn't working on frames > > + * bigger than 1514 bytes. Perhaps the buffer is only 1518 > > + * bytes fitting a normal frame and a checksum? > > + * Just use software checksumming and bypass on bigger frames. > > + */ > > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > > + ret = skb_checksum_help(skb); > > + if (ret) > > + return ret; > > + } > > + word1 |= TSS_BYPASS_BIT; > > + } else if (skb->ip_summed == CHECKSUM_PARTIAL) { > > int tcp = 0; > > > > if (skb->protocol == htons(ETH_P_IP)) { > > [ context: tag_rtl4_a.c is a "category 2", aka "Ethertype", tagger ] > > We say this in Documentation/networking/dsa/dsa.rst: > > Checksum offload should work with category 1 and 2 taggers when the DSA conduit > driver declares NETIF_F_HW_CSUM in vlan_features > and looks at csum_start and csum_offset. > For those cases, DSA will shift the checksum start and offset by > the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM > or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the > offload hardware already expects that specific tag (perhaps due to matching > vendors). Since things work smoothly I can only assume that the Gemini checksum engine actually knows about the Realtek ethertype (0x8899) and the protocol (0xa) and takes action on that, since the switch works. OR: it has some heuristic on for how to handle it. (Such as looking for a valid TCP or UDP header to figure out where to put the checksum.) But I have no idea how it does it. It doesn't have a firmware AFAIK. Examples listed were ICMP so just IP checksums but I tried for example SSH, and HTTP and packets look like this: 22:51:35.457191 9a:ec:30:5a:46:96 (oui Unknown) > bc:ae:c5:6b:a8:3d (oui Unknown), ethertype IPv4 (0x0800), length 434: (tos 0x48, ttl 64, id 8221, offset 0, flags [DF], proto TCP (6), length 420) _gateway.48102 > fedora.ssh: Flags [P.], cksum 0xcf1b (correct), seq 811:1179, ack 2310, win 2054, options [nop,nop,TS val 74858741 ecr 1981407207], length 368 Checksum correct. So... > DSA user ports inherit those flags from the conduit, and it is up to > the driver to correctly fall back to software checksum when the IP header is not > where the hardware expects. If that check is ineffective, the packets might go > to the network without a proper checksum (the checksum field will have the > pseudo IP header sum). It definately does not contain the pseudo IP header sum because it would be the same all the time but tcpdump is happy: cksum 0xcf1b (correct) cksum 0x0655 (correct) cksum 0xd247 (correct) cksum 0x06b1 (correct) > Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol, > rather than depending on skb->len?! > > if (skb->protocol == htons(ETH_P_IP)) { > word1 |= TSS_IP_CHKSUM_BIT; > tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; > } else { /* IPv6 */ > word1 |= TSS_IPV6_ENABLE_BIT; > tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP; > } // here > word1 |= TSS_BYPASS_BIT; Oddly it assumes everything is either TCP or UDP on IPv4 or IPv6. And yet things such as ICMP work just fine. I think the checksum engine can contain some various heuristics, such as if it cannot recognize what is coming in as the selected TCP or UDP, it will pass right through. > Gemini should never attempt to provide checksums for DSA-tagged packets > unless it is able to take skb->csum_start into consideration, otherwise > it will get it wrong. > > This is somewhat independent of the other problem you've found, which > seems to be that large non-DSA packets get truncated anyway. But not > bypassing TX checksum offload truncates a packet? Hmm, strange. I have a theory about that in the comment, which is that when they engineered the hardware they only put in a hardware buffer for 1518 bytes in the checksum engine. If you try to put in any more it gets truncated. It's a reasonable guess. If you do not set the checkumming engine to "bypass" it will try to fit the incoming paket into the checksumming buffer, and then it will look to see if it can find the right TCP or UDP headers. If it can't it will pass the packet out from the buffer without doing any changes. But the buffer is just 1518 bytes, which means that no matter what kind of package it is, it will get truncated if it does not fit into the checksumming buffer. This would give exactly the behaviour we're seeing. Yours, Linus Walleij
On Mon, Nov 06, 2023 at 11:44:37PM +0100, Linus Walleij wrote: > That indeed seems like the intention. But it's a bit suboptimal, because it > disables hardware checksum just because the MTU goes over a > certain level, and stops using the hardware checksum also for all > packets smaller than the MTU :( > > I'll delete this and make the driver slot in the SW fallback per-packet > instead, I think it is fair to assume that most packets will be < MTU > and it is really just a question of where the fallback gets called. Performing the software checksum per packet instead of doing it for all packets seems more sensible. I haven't looked at the other offload bits from GMAC_OFFLOAD_FEATURES to determine if changing those is going to be problematic. > > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > > > > To be clear, I didn't suggest any of this. I just pointed towards the gemini.c > > driver as being the problem. Please remove my Suggested-by tag. > > OK sorry, it was just my way of trying to provide credit where > credit is due, because you helped so much with this bug. Ok, but asserting the validity of the DSA code and vaguely pointing towards an unrelated driver and not even specifying what might be wrong with its xmit procedure still does not count as worth a Suggested-by tag to me. So still, please remove it. > > We say this in Documentation/networking/dsa/dsa.rst: > > > > Checksum offload should work with category 1 and 2 taggers when the DSA conduit > > driver declares NETIF_F_HW_CSUM in vlan_features > > and looks at csum_start and csum_offset. > > For those cases, DSA will shift the checksum start and offset by > > the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM > > or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the > > offload hardware already expects that specific tag (perhaps due to matching > > vendors). > > Since things work smoothly I can only assume that the Gemini > checksum engine actually knows about the Realtek ethertype (0x8899) > and the protocol (0xa) and takes action on that, since the switch works. > > OR: it has some heuristic on for how to handle it. (Such as looking for > a valid TCP or UDP header to figure out where to put the checksum.) The second option seems more plausible to me. It seems unlikely that a non-Realtek controller would have information about a Realtek proprietary protocol, that even Linux people have trouble finding a lot of information about, when explicitly trying to do so. > But I have no idea how it does it. It doesn't have a firmware AFAIK. > > Examples listed were ICMP so just IP checksums but I tried for example > SSH, and HTTP and packets look like this: > > 22:51:35.457191 9a:ec:30:5a:46:96 (oui Unknown) > bc:ae:c5:6b:a8:3d > (oui Unknown), > ethertype IPv4 (0x0800), length 434: (tos 0x48, ttl 64, id 8221, > offset 0, flags [DF], proto TCP (6), length 420) > _gateway.48102 > fedora.ssh: Flags [P.], cksum 0xcf1b (correct), > seq 811:1179, ack 2310, > win 2054, options [nop,nop,TS val 74858741 ecr 1981407207], length 368 > > Checksum correct. So... Well, the checksum is correct because it's done in software by the network stack, prior to dsa_user_xmit() being even called, and thus prior to the DSA tag being added. "ethtool -k lan0 | grep sum" will tell you as much (compare it to "ethtool -k eth0" which will list some offload bits set). > > Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol, > > rather than depending on skb->len?! > > > > if (skb->protocol == htons(ETH_P_IP)) { > > word1 |= TSS_IP_CHKSUM_BIT; > > tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; > > } else { /* IPv6 */ > > word1 |= TSS_IPV6_ENABLE_BIT; > > tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP; > > } // here > > word1 |= TSS_BYPASS_BIT; > > Oddly it assumes everything is either TCP or UDP on > IPv4 or IPv6. And yet things such as ICMP work just fine. > > I think the checksum engine can contain some various > heuristics, such as if it cannot recognize what is coming > in as the selected TCP or UDP, it will pass right through. > > > Gemini should never attempt to provide checksums for DSA-tagged packets > > unless it is able to take skb->csum_start into consideration, otherwise > > it will get it wrong. > > > > This is somewhat independent of the other problem you've found, which > > seems to be that large non-DSA packets get truncated anyway. But not > > bypassing TX checksum offload truncates a packet? Hmm, strange. > > I have a theory about that in the comment, which is that when they > engineered the hardware they only put in a hardware buffer for > 1518 bytes in the checksum engine. If you try to put in any more > it gets truncated. It's a reasonable guess. > > If you do not set the checkumming engine to "bypass" it will try > to fit the incoming paket into the checksumming buffer, and then > it will look to see if it can find the right TCP or UDP headers. > If it can't it will pass the packet out from the buffer without doing > any changes. But the buffer is just 1518 bytes, which means that > no matter what kind of package it is, it will get truncated if it > does not fit into the checksumming buffer. > > This would give exactly the behaviour we're seeing. The evidence you've presented seems to support your position and not mine. The controller does not attempt to provide checksums for DSA-tagged packets, that is not a problem because those have checksums already, but the packet size is the actual problem, irrespective of whether the packets are DSA-tagged or not. But in that case I have some comments on your latest version of the patch set.
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c index 576174a862a9..84295c1b87e6 100644 --- a/drivers/net/ethernet/cortina/gemini.c +++ b/drivers/net/ethernet/cortina/gemini.c @@ -1145,6 +1145,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, dma_addr_t mapping; unsigned short mtu; void *buffer; + int ret; mtu = ETH_HLEN; mtu += netdev->mtu; @@ -1165,7 +1166,19 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, word3 |= mtu; } - if (skb->ip_summed != CHECKSUM_NONE) { + if (skb->len >= ETH_FRAME_LEN) { + /* Hardware offloaded checksumming isn't working on frames + * bigger than 1514 bytes. Perhaps the buffer is only 1518 + * bytes fitting a normal frame and a checksum? + * Just use software checksumming and bypass on bigger frames. + */ + if (skb->ip_summed == CHECKSUM_PARTIAL) { + ret = skb_checksum_help(skb); + if (ret) + return ret; + } + word1 |= TSS_BYPASS_BIT; + } else if (skb->ip_summed == CHECKSUM_PARTIAL) { int tcp = 0; if (skb->protocol == htons(ETH_P_IP)) {
The Gemini ethernet controller provides hardware checksumming for frames up to 1514 bytes including ethernet headers but not FCS. If we start sending bigger frames (after first bumping up the MTU on both interfaces sending and receiveing the frames), truncated packets start to appear on the target such as in this tcpdump resulting from ping -s 1474: 23:34:17.241983 14:d6:4d:a8:3c:4f (oui Unknown) > bc:ae:c5:6b:a8:3d (oui Unknown), ethertype IPv4 (0x0800), length 1514: truncated-ip - 2 bytes missing! (tos 0x0, ttl 64, id 32653, offset 0, flags [DF], proto ICMP (1), length 1502) OpenWrt.lan > Fecusia: ICMP echo request, id 1672, seq 50, length 1482 If we bypass the hardware checksumming and provide a software fallback, everything starts working fine up to the max TX MTU of 2047 bytes, for example ping -s2000 192.168.1.2: 00:44:29.587598 bc:ae:c5:6b:a8:3d (oui Unknown) > 14:d6:4d:a8:3c:4f (oui Unknown), ethertype IPv4 (0x0800), length 2042: (tos 0x0, ttl 64, id 51828, offset 0, flags [none], proto ICMP (1), length 2028) Fecusia > OpenWrt.lan: ICMP echo reply, id 1683, seq 4, length 2008 The bit enabling to bypass hardware checksum (or any of the "TSS" bits) are undocumented in the hardware reference manual. The entire hardware checksum unit appears undocumented. The conclusion that we need to use the "bypass" bit was found by trial-and-error. Since no hardware checksum will happen, we slot in a software checksum fallback. Check for the condition where we need to compute checksum on the skb with either hardware or software using == CHECKSUM_PARTIAL instead of != CHECKSUM_NONE which is an incomplete check according to <linux/skbuff.h>. On the D-Link DIR-685 router this fixes a bug on the conduit interface to the RTL8366RB DSA switch: as the switch needs to add space for its tag it increases the MTU on the conduit interface to 1504 and that means that when the router sends packages of 1500 bytes these get an extra 4 bytes of DSA tag and the transfer fails because of the erroneous hardware checksumming, affecting such basic functionality as the LuCI web interface. Suggested-by: Vladimir Oltean <olteanv@gmail.com> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/net/ethernet/cortina/gemini.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)