Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum

[Vladimir Oltean]

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@xxxxxx>
> > > ---
> > 
> > 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.