| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89iJLfxng1sYL5Zk0mknXpyYQPCp83m3KgD2KJ2_hKCpEUg@mail.gmail.com>
Date: Fri, 15 Dec 2023 12:17:57 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Hans Ulli Kroll <ulli.kroll@...glemail.com>, "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: ethernet: cortina: Drop software checksumming
On Fri, Dec 15, 2023 at 10:32 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Dec 15, 2023 at 9:49 AM Linus Walleij <linus.walleij@...aro.org> wrote:
> >
> > The recent change to allow large frames without hardware checksumming
> > slotted in software checksumming in the driver if hardware could not
> > do it.
> >
> > This will however upset TSO (TCP Segment Offloading). Typical
> > error dumps includes this:
> >
> > skb len=2961 headroom=222 headlen=66 tailroom=0
> > (...)
> > WARNING: CPU: 0 PID: 956 at net/core/dev.c:3259 skb_warn_bad_offload+0x7c/0x108
> > gemini-ethernet-port: caps=(0x0000010000154813, 0x00002007ffdd7889)
> >
> > And the packets do not go through.
> >
> > After investigating I drilled it down to the introduction of the
> > software checksumming in the driver.
> >
> > Since the segmenting of packets will be done by the hardware this
> > makes a bit of sense since in that case the hardware also needs to
> > be keeping track of the checksumming.
> >
> > That begs the question why large TCP or UDP packets also have to
> > bypass the checksumming (like e.g. ICMP does). If the hardware is
> > splitting it into smaller packets per-MTU setting, and checksumming
> > them, why is this happening then? I don't know. I know it is needed,
> > from tests: the OpenWrt webserver uhttpd starts sending big skb:s (up
> > to 2047 bytes, the max MTU) and above 1514 bytes it starts to fail
> > and hang unless the bypass bit is set: the frames are not getting
> > through.
> >
> > Keeping the size check but removing the software checksum makes things
> > work again. This was probably dubious to introduce in the first place.
> >
> > Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames")
> > Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> > ---
> > drivers/net/ethernet/cortina/gemini.c | 8 --------
> > 1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> > index 78287cfcbf63..255fcffc1579 100644
> > --- a/drivers/net/ethernet/cortina/gemini.c
> > +++ b/drivers/net/ethernet/cortina/gemini.c
> > @@ -1145,7 +1145,6 @@ 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;
> > @@ -1166,14 +1165,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
> > * checksum buffer is only 1518 bytes, so when the frames get
> > * bigger they get truncated, or the last few bytes get
> > * overwritten by the FCS.
> > - *
> > - * Just use software checksumming and bypass on bigger frames.
> > */
> > - if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > - ret = skb_checksum_help(skb);
> > - if (ret)
> > - return ret;
> > - }
>
> If the hardware does not support checksumming for frames bigger than
> ETH_FRAME_LEN,
> then an appropriate mitigation would be to have an ndo_features_check() ?
>
> Depending on packet being gso or not, you would have to check skb->len
> or shinfo->gso_size
>
> The ndo_features_check could then take a more appropriate action
> (forcing GSO, and/or forcing software checksumming)
>
> This driver claims to support TSO, but I do not see it using
> shinfo->gso_size, something must be very wrong...
>
> I would simply remove this TSO part, before the driver really supports
> TSO properly.
>
> diff --git a/drivers/net/ethernet/cortina/gemini.c
> b/drivers/net/ethernet/cortina/gemini.c
> index 78287cfcbf6388f01bfab417c264f41f3a1a16f2..829cb69982fe1caf99b56363e9e0565fbaecc82e
> 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -79,8 +79,7 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT)
>
> #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \
> - NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \
> - NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6)
> + NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM )
>
> /**
> * struct gmac_queue_page - page buffer per-page info
I do not have the datasheet for this NIC, but my naive attempt would
be something like:
diff --git a/drivers/net/ethernet/cortina/gemini.c
b/drivers/net/ethernet/cortina/gemini.c
index 78287cfcbf6388f01bfab417c264f41f3a1a16f2..3c902dac16f7d539349178ebc4a49c7e48edced5
100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1143,39 +1143,18 @@ static int gmac_map_tx_bufs(struct net_device
*netdev, struct sk_buff *skb,
struct gmac_txdesc *txd;
skb_frag_t *skb_frag;
dma_addr_t mapping;
- unsigned short mtu;
void *buffer;
int ret;
- mtu = ETH_HLEN;
- mtu += netdev->mtu;
- if (skb->protocol == htons(ETH_P_8021Q))
- mtu += VLAN_HLEN;
-
word1 = skb->len;
word3 = SOF_BIT;
- if (word1 > mtu) {
+ if (skb_is_gso(skb)) {
word1 |= TSS_MTU_ENABLE_BIT;
- word3 |= mtu;
+ word3 |= skb_shinfo(skb)->gso_size;
}
- if (skb->len >= ETH_FRAME_LEN) {
- /* Hardware offloaded checksumming isn't working on frames
- * bigger than 1514 bytes. A hypothesis about this is that the
- * checksum buffer is only 1518 bytes, so when the frames get
- * bigger they get truncated, or the last few bytes get
- * overwritten by the FCS.
- *
- * 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) {
+ if (skb->ip_summed == CHECKSUM_PARTIAL) {
int tcp = 0;
/* We do not switch off the checksumming on non TCP/UDP
Powered by blists - more mailing lists