Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>,
	woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
	vivien.didelot@gmail.com, f.fainelli@gmail.com,
	davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
Date: Thu, 15 Jul 2021 09:54:55 +0300	[thread overview]
Message-ID: <20210715065455.7nu7zgle2haa6wku@skbuf> (raw)
In-Reply-To: <YO9F2LhTizvr1l11@lunn.ch>

On Wed, Jul 14, 2021 at 10:15:20PM +0200, Andrew Lunn wrote:
> On Wed, Jul 14, 2021 at 10:48:12PM +0300, Vladimir Oltean wrote:
> > Hi Lino,
> > 
> > On Wed, Jul 14, 2021 at 09:17:23PM +0200, Lino Sanfilippo wrote:
> > > If the checksum calculation is offloaded to the network device (e.g due to
> > > NETIF_F_HW_CSUM inherited from the DSA master device), the calculated
> > > layer 4 checksum is incorrect. This is since the DSA tag which is placed
> > > after the layer 4 data is seen as a part of the data portion and thus
> > > errorneously included into the checksum calculation.
> > > To avoid this, always calculate the layer 4 checksum in software.
> > > 
> > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> > > ---
> > 
> > This needs to be solved more generically for all tail taggers. Let me
> > try out a few things tomorrow and come with a proposal.
> 
> Maybe the skb_linearize() is also a generic problem, since many of the
> tag drivers are using skb_put()? It looks like skb_linearize() is
> cheap because checking if the skb is already linear is cheap. So maybe
> we want to do it unconditionally?

Yeah, but we should let the stack deal with both issues in validate_xmit_skb().
There is a skb_needs_linearize() call which returns false because the
DSA interface inherits NETIF_F_SG from the master via dsa_slave_create():

	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;

Arguably that's the problem right there, we shouldn't inherit neither
NETIF_F_HW_CSUM nor NETIF_F_SG from the list of features inheritable by
8021q uppers.

- If we inherit NETIF_F_SG we obligate ourselves to call skb_linearize()
  for tail taggers on xmit. DSA probably doesn't break anything for
  header taggers though even if the xmit skb is paged, since the DSA
  header would always be part of the skb head, so this is a feature we
  could keep for them.
- If we inherit NETIF_F_HW_CSUM from the master for tail taggers, it is
  actively detrimential to keep this feature enabled, as proven my Lino.
  As for header taggers, I fail to see how this would be helpful, since
  the DSA master would always fail to see the real IP header (it has
  been pushed to the right by the DSA tag), and therefore, the DSA
  master offload would be effectively bypassed. So no point, really, in
  inheriting it in the first place in any situation.

Lino, to fix these bugs by letting validate_xmit_skb() know what works
for DSA and what doesn't, could you please:

(a) move the current slave_dev->features assignment to
    dsa_slave_setup_tagger()? We now support changing the tagging
    protocol at runtime, and everything that depends on what the tagging
    protocol is (in this case, tag_ops->needed_headroom vs
    tag_ops->needed_tailroom) should be put in that function.
(b) unconditionally clear NETIF_F_HW_CSUM from slave_dev->features,
    after inheriting the vlan_features from the master?
(c) clear NETIF_F_SG from slave_dev->features if we have a non-zero
    tag_ops->needed_tailroom?

Thanks.

By the way I didn't get the chance to test anything, sorry if there is
any mistake in my reasoning.

  reply	other threads:[~2021-07-15  6:55 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-14 19:17 [PATCH 0/2] Fixes for KSZ DSA switch Lino Sanfilippo
2021-07-14 19:17 ` [PATCH 1/2] net: dsa: tag_ksz: linearize SKB before adding DSA tag Lino Sanfilippo
2021-07-14 19:17 ` [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum Lino Sanfilippo
2021-07-14 19:48   ` Vladimir Oltean
2021-07-14 20:15     ` Andrew Lunn
2021-07-15  6:54       ` Vladimir Oltean [this message]
2021-07-15 11:16         ` Aw: " Lino Sanfilippo
2021-07-15 11:49           ` Vladimir Oltean
2021-07-15 13:04             ` Aw: " Lino Sanfilippo
2021-07-15 13:12               ` Vladimir Oltean
2021-07-15 13:34                 ` Aw: " Lino Sanfilippo
2021-07-15 13:08         ` Andrew Lunn
2021-07-15 14:36           ` Vladimir Oltean
2021-07-15 15:50             ` Andrew Lunn
2021-07-19  8:20             ` Aw: " Lino Sanfilippo
2021-07-19  8:24               ` Vladimir Oltean
2021-07-15 16:24           ` Florian Fainelli
2021-07-15 23:23 ` [PATCH 0/2] Fixes for KSZ DSA switch Florian Fainelli
2021-07-16  8:49   ` Aw: " Lino Sanfilippo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210715065455.7nu7zgle2haa6wku@skbuf \
    --to=olteanv@gmail.com \
    --cc=LinoSanfilippo@gmx.de \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vivien.didelot@gmail.com \
    --cc=woojung.huh@microchip.com \
    --subject='Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).