These patches fix issues I encountered while using a KSZ9897 as a DSA
switch with a broadcom GENET network device as the DSA master device.
PATCH 1 fixes an invalid access to an SKB in case it is scattered.
PATCH 2 fixes incorrect hardware checksum calculation caused by the DSA
tag.
The patches have been tested with a KSZ9897 and apply against net-next.
Lino Sanfilippo (2):
net: dsa: tag_ksz: linearize SKB before adding DSA tag
net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
net/dsa/tag_ksz.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
base-commit: 5e437416ff66981d8154687cfdf7de50b1d82bfc
--
2.32.0
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 <[email protected]>
---
net/dsa/tag_ksz.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 364f509d7cd7..d59f0e7019e2 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -56,6 +56,9 @@ static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
if (skb_linearize(skb))
return NULL;
+ if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+ return NULL;
+
/* Tag encoding */
tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
addr = skb_mac_header(skb);
@@ -120,6 +123,9 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
if (skb_linearize(skb))
return NULL;
+ if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+ return NULL;
+
/* Tag encoding */
tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN);
addr = skb_mac_header(skb);
@@ -173,6 +179,9 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
if (skb_linearize(skb))
return NULL;
+ if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
+ return NULL;
+
/* Tag encoding */
tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
addr = skb_mac_header(skb);
--
2.32.0
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 <[email protected]>
> ---
This needs to be solved more generically for all tail taggers. Let me
try out a few things tomorrow and come with a proposal.
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 <[email protected]>
> > ---
>
> 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?
Andrew
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 <[email protected]>
> > > ---
> >
> > 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.
Hi Vladimir,
> Gesendet: Donnerstag, 15. Juli 2021 um 08:54 Uhr
> Von: "Vladimir Oltean" <[email protected]>
> An: "Andrew Lunn" <[email protected]>
> Cc: "Lino Sanfilippo" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: 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 <[email protected]>
> > > > ---
> > >
> > > 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?
>
Sure, I will test this solution. But I think NETIF_F_FRAGLIST should also be
cleared in this case, right?
Regards,
Lino
On Thu, Jul 15, 2021 at 01:16:12PM +0200, Lino Sanfilippo wrote:
> Sure, I will test this solution. But I think NETIF_F_FRAGLIST should also be
> cleared in this case, right?
Hmm, interesting question. I think only hns3 makes meaningful use of
NETIF_F_FRAGLIST, right? I'm looking at hns3_fill_skb_to_desc().
Other drivers seem to set it for ridiculous reasons - looking at commit
66aa0678efc2 ("ibmveth: Support to enable LSO/CSO for Trunk VEA.") -
they set NETIF_F_FRAGLIST and then linearize the skb chain anyway. The
claimed 4x throughput benefit probably has to do with less skbs
traversing the stack? I don't know.
Anyway, it is hard to imagine all the things that could go wrong with
chains of IP fragments on a DSA interface, precisely because I have so
few examples to look at. I would say, header taggers are probably fine,
tail taggers not so much, so apply the same treatment as for NETIF_F_SG?
> Gesendet: Donnerstag, 15. Juli 2021 um 13:49 Uhr
> Von: "Vladimir Oltean" <[email protected]>
> An: "Lino Sanfilippo" <[email protected]>
> Cc: "Andrew Lunn" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
>
> On Thu, Jul 15, 2021 at 01:16:12PM +0200, Lino Sanfilippo wrote:
> > Sure, I will test this solution. But I think NETIF_F_FRAGLIST should also be
> > cleared in this case, right?
>
> Hmm, interesting question. I think only hns3 makes meaningful use of
> NETIF_F_FRAGLIST, right? I'm looking at hns3_fill_skb_to_desc().
> Other drivers seem to set it for ridiculous reasons - looking at commit
> 66aa0678efc2 ("ibmveth: Support to enable LSO/CSO for Trunk VEA.") -
> they set NETIF_F_FRAGLIST and then linearize the skb chain anyway. The
> claimed 4x throughput benefit probably has to do with less skbs
> traversing the stack? I don't know.
>
> Anyway, it is hard to imagine all the things that could go wrong with
> chains of IP fragments on a DSA interface, precisely because I have so
> few examples to look at. I would say, header taggers are probably fine,
> tail taggers not so much, so apply the same treatment as for NETIF_F_SG?
>
Please note that skb_put() asserts that the SKB is linearized. So I think we
should rather clear both NETIF_F_FRAGLIST and NETIF_F_SG unconditionally since also
header taggers use some form of skb_put() dont they?
> - 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.
The Marvell MACs know about DSA and should be able to perform hardware
checksumming. It is a long time since i looked at how this works, but
i think there is a field in the descriptor which gets set with the
offset to the IP header, so it work for DSA as well as EDSA.
I _think_ Broadcom MACs also know about Broadcom tags and can do the
right thing.
So we need to be a bit careful here to prevent performance regressions
for same vendor MAC+Switch combinations.
Andrew
On Thu, Jul 15, 2021 at 03:04:31PM +0200, Lino Sanfilippo wrote:
> Please note that skb_put() asserts that the SKB is linearized. So I think we
> should rather clear both NETIF_F_FRAGLIST and NETIF_F_SG unconditionally since also
> header taggers use some form of skb_put() dont they?
The tail taggers use skb_put() as part of the routine to make room for
the tail tag.
Some of the header taggers use __skb_put_padto() when the packets are
too small (under ETH_ZLEN). When they are so small they are definitely
linear already.
We don't have a third form/use of skb_put().
> Gesendet: Donnerstag, 15. Juli 2021 um 15:12 Uhr
> Von: "Vladimir Oltean" <[email protected]>
> An: "Lino Sanfilippo" <[email protected]>
> Cc: "Andrew Lunn" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
>
> On Thu, Jul 15, 2021 at 03:04:31PM +0200, Lino Sanfilippo wrote:
> > Please note that skb_put() asserts that the SKB is linearized. So I think we
> > should rather clear both NETIF_F_FRAGLIST and NETIF_F_SG unconditionally since also
> > header taggers use some form of skb_put() dont they?
>
> The tail taggers use skb_put() as part of the routine to make room for
> the tail tag.
>
> Some of the header taggers use __skb_put_padto() when the packets are
> too small (under ETH_ZLEN). When they are so small they are definitely
> linear already.
>
> We don't have a third form/use of skb_put().
>
Ah ok, I see. Then it should be fine do clear both flags only in case of
tail taggers.
Regards,
Lino
On Thu, Jul 15, 2021 at 03:08:53PM +0200, Andrew Lunn wrote:
> > - 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.
>
> The Marvell MACs know about DSA and should be able to perform hardware
> checksumming. It is a long time since i looked at how this works, but
> i think there is a field in the descriptor which gets set with the
> offset to the IP header, so it work for DSA as well as EDSA.
>
> I _think_ Broadcom MACs also know about Broadcom tags and can do the
> right thing.
>
> So we need to be a bit careful here to prevent performance regressions
> for same vendor MAC+Switch combinations.
Tell me more (show me some code). Do Marvell Ethernet controllers which
support TX checksumming with Marvell switches do different things
depending on whether DSA or EDSA is used? Because we can currently
toggle between DSA and EDSA at runtime.
This new information means we can only accept Lino's patch 2/2 as-is for
the "net" tree, otherwise we will introduce regressions one way or
another. It will only be a partial fix for the particular case of KSZ
switches which probably have no DSA master counterpart to support TX
checksumming.
I expect Marvell switches to be equally broken on the Broadcom genet
controller? No one will provide the TX checksum in that case. And that
is not even "fixable" without devising a system where custom code can be
run per {tagger, DSA master} pair, and this includes the case where the
tagging protocol changes at runtime.
On 7/15/21 6:08 AM, Andrew Lunn wrote:
>> - 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.
>
> The Marvell MACs know about DSA and should be able to perform hardware
> checksumming. It is a long time since i looked at how this works, but
> i think there is a field in the descriptor which gets set with the
> offset to the IP header, so it work for DSA as well as EDSA.
>
> I _think_ Broadcom MACs also know about Broadcom tags and can do the
> right thing.
Yes, the SYSTEMPORT MAC as well as bgmac to some extent will
automagically work, but they have to be told to expect a Broadcom tag in
order to generate an appropriate checksum on receive. This is why we
have this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/broadcom/bcmsysport.c#n144
Likewise, for TX:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/broadcom/bcmsysport.c#n172
--
Florian
> Tell me more (show me some code).
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/marvell/mvneta.c#L1747
and
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/marvell/mvneta.c#L1944
It uses skb_network_offset(skb) to know where the real header is. This
should work independent of DSA or EDSA.
mvpp2_main.c looks to have something similar. The older mv643xx_eth.c
also has something, but it is more subtle. Ah, found it:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/marvell/mv643xx_eth.c#L683
> I expect Marvell switches to be equally broken on the Broadcom genet
> controller?
Maybe. Depends on how genet works. A Broadcom switch connected to a
Marvell MAC probably works, since the code is generic. It should work
for any switch which uses head tagging, although mv643xx_eth.c is
limited to 4 or 8 byte tags.
Andrew
On 7/14/21 12:17 PM, Lino Sanfilippo wrote:
> These patches fix issues I encountered while using a KSZ9897 as a DSA
> switch with a broadcom GENET network device as the DSA master device.
>
Is this off the shelf hardware that can be interfaced to a Raspberry Pi
4 or is this a custom design that only you have access to?
> PATCH 1 fixes an invalid access to an SKB in case it is scattered.
> PATCH 2 fixes incorrect hardware checksum calculation caused by the DSA
> tag.
>
> The patches have been tested with a KSZ9897 and apply against net-next.
>
> Lino Sanfilippo (2):
> net: dsa: tag_ksz: linearize SKB before adding DSA tag
> net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
>
> net/dsa/tag_ksz.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>
> base-commit: 5e437416ff66981d8154687cfdf7de50b1d82bfc
>
--
Florian
Hi Florian,
> Gesendet: Freitag, 16. Juli 2021 um 01:23 Uhr
> Von: "Florian Fainelli" <[email protected]>
> An: "Lino Sanfilippo" <[email protected]>, [email protected]
> Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: Re: [PATCH 0/2] Fixes for KSZ DSA switch
>
> On 7/14/21 12:17 PM, Lino Sanfilippo wrote:
> > These patches fix issues I encountered while using a KSZ9897 as a DSA
> > switch with a broadcom GENET network device as the DSA master device.
> >
>
> Is this off the shelf hardware that can be interfaced to a Raspberry Pi
> 4 or is this a custom design that only you have access to?
>
The setup I use is a Raspberry Pi 4 connected to an EVB KSZ9897 from Microchip:
https://www.microchip.com/DevelopmentTools/ProductDetails/PartNO/EVB-KSZ9897-1
Best regards,
Lino
Hi,
> Gesendet: Donnerstag, 15. Juli 2021 um 16:36 Uhr
> Von: "Vladimir Oltean" <[email protected]>
> An: "Andrew Lunn" <[email protected]>
> Cc: "Lino Sanfilippo" <[email protected]>, [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
> Betreff: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
>
> On Thu, Jul 15, 2021 at 03:08:53PM +0200, Andrew Lunn wrote:
> > > - 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.
> >
> > The Marvell MACs know about DSA and should be able to perform hardware
> > checksumming. It is a long time since i looked at how this works, but
> > i think there is a field in the descriptor which gets set with the
> > offset to the IP header, so it work for DSA as well as EDSA.
> >
> > I _think_ Broadcom MACs also know about Broadcom tags and can do the
> > right thing.
> >
> > So we need to be a bit careful here to prevent performance regressions
> > for same vendor MAC+Switch combinations.
>
> Tell me more (show me some code). Do Marvell Ethernet controllers which
> support TX checksumming with Marvell switches do different things
> depending on whether DSA or EDSA is used? Because we can currently
> toggle between DSA and EDSA at runtime.
>
> This new information means we can only accept Lino's patch 2/2 as-is for
> the "net" tree, otherwise we will introduce regressions one way or
> another. It will only be a partial fix for the particular case of KSZ
> switches which probably have no DSA master counterpart to support TX
> checksumming.
>
Should I then resend the series with patch 1 handling the NETIF_F_SG and
NETIF_F_FRAGLIST flags (i.e. deleting them if tailroom is needed) in
dsa_slave_setup_tagger() as you suggested and patch 2 as it is?
Regards,
Lino
On Mon, Jul 19, 2021 at 10:20:13AM +0200, Lino Sanfilippo wrote:
> Should I then resend the series with patch 1 handling the NETIF_F_SG and
> NETIF_F_FRAGLIST flags (i.e. deleting them if tailroom is needed) in
> dsa_slave_setup_tagger() as you suggested and patch 2 as it is?
Yup. The TX checksum offload flag on DSA interfaces is definitely
net-next material if we want to do it properly, so we can revisit it
there. Thanks.