Messages in this thread | | | Date | Thu, 15 Jul 2021 09:54:55 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum |
| |
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.
| |