[PATCH v2 0/2] Fixes for KSZ DSA switch

[PATCH v2 0/2] Fixes for KSZ DSA switch

[Lino Sanfilippo]

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.

Changes in v2:
- instead of linearizing the SKBs only for KSZ switches ensure linearized
  SKBs for all tail taggers by clearing the feature flags NETIF_F_HW_SG and
  NETIF_F_FRAGLIST (suggested by Vladimir Oltean)

The patches have been tested with a KSZ9897 and apply against net-next.

Lino Sanfilippo (2):
  net: dsa: ensure linearized SKBs in case of tail taggers
  net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum

 net/dsa/slave.c   | 14 +++++++++-----
 net/dsa/tag_ksz.c |  9 +++++++++
 2 files changed, 18 insertions(+), 5 deletions(-)


base-commit: 54cb43199e14c1181ddcd4a3782f1f7eb56bdab8
-- 
2.32.0

[PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Lino Sanfilippo]

The function skb_put() that is used by tail taggers to make room for the
DSA tag must only be called for linearized SKBS. However in case that the
slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
SKB passed to the slaves transmit function may not be linearized.
Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
for tail taggers.
Furthermore since the tagging protocol can be changed at runtime move the
code for setting up the slaves features into dsa_slave_setup_tagger().

Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 net/dsa/slave.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 22ce11cd770e..ae2a648ed9be 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 	struct dsa_slave_priv *p = netdev_priv(slave);
 	const struct dsa_port *cpu_dp = dp->cpu_dp;
 	struct net_device *master = cpu_dp->master;
+	const struct dsa_switch *ds = dp->ds;
 
 	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
 	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
@@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 	slave->needed_tailroom += master->needed_tailroom;
 
 	p->xmit = cpu_dp->tag_ops->xmit;
+
+	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
+		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+	slave->hw_features |= NETIF_F_HW_TC;
+	slave->features |= NETIF_F_LLTX;
+	if (slave->needed_tailroom)
+		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
 }
 
 static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
@@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
-	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
-		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
-	slave_dev->hw_features |= NETIF_F_HW_TC;
-	slave_dev->features |= NETIF_F_LLTX;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	if (!is_zero_ether_addr(port->mac))
 		ether_addr_copy(slave_dev->dev_addr, port->mac);
-- 
2.32.0

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Vladimir Oltean]

On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
> The function skb_put() that is used by tail taggers to make room for the
> DSA tag must only be called for linearized SKBS. However in case that the
> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
> SKB passed to the slaves transmit function may not be linearized.
> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
> for tail taggers.
> Furthermore since the tagging protocol can be changed at runtime move the
> code for setting up the slaves features into dsa_slave_setup_tagger().
> 
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  net/dsa/slave.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 22ce11cd770e..ae2a648ed9be 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>  	struct dsa_slave_priv *p = netdev_priv(slave);
>  	const struct dsa_port *cpu_dp = dp->cpu_dp;
>  	struct net_device *master = cpu_dp->master;
> +	const struct dsa_switch *ds = dp->ds;
>  
>  	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
>  	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>  	slave->needed_tailroom += master->needed_tailroom;
>  
>  	p->xmit = cpu_dp->tag_ops->xmit;
> +
> +	slave->features = master->vlan_features | NETIF_F_HW_TC;
> +	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> +		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +	slave->hw_features |= NETIF_F_HW_TC;
> +	slave->features |= NETIF_F_LLTX;
> +	if (slave->needed_tailroom)
> +		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
>  }
>  
>  static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
>  	if (slave_dev == NULL)
>  		return -ENOMEM;
>  
> -	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
> -	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
> -		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> -	slave_dev->hw_features |= NETIF_F_HW_TC;
> -	slave_dev->features |= NETIF_F_LLTX;
>  	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>  	if (!is_zero_ether_addr(port->mac))
>  		ether_addr_copy(slave_dev->dev_addr, port->mac);
> -- 
> 2.32.0
> 

I would have probably changed the code in dsa_slave_create just like
this:

-	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	slave->features = NETIF_F_HW_TC;
...
-	slave_dev->vlan_features = master->vlan_features;

and in dsa_slave_setup_tagger:

+	vlan_features = master->vlan_features;
+	slave->features &= ~vlan_features;
+	if (slave->needed_tailroom)
+		vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
+	slave->features |= vlan_features;
+	slave->vlan_features = vlan_features;

no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?

And I would probably add:

Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Florian Fainelli]

On 7/21/2021 4:35 PM, Vladimir Oltean wrote:
> On Wed, Jul 21, 2021 at 11:56:41PM +0200, Lino Sanfilippo wrote:
>> The function skb_put() that is used by tail taggers to make room for the
>> DSA tag must only be called for linearized SKBS. However in case that the
>> slave device inherited features like NETIF_F_HW_SG or NETIF_F_FRAGLIST the
>> SKB passed to the slaves transmit function may not be linearized.
>> Avoid those SKBs by clearing the NETIF_F_HW_SG and NETIF_F_FRAGLIST flags
>> for tail taggers.
>> Furthermore since the tagging protocol can be changed at runtime move the
>> code for setting up the slaves features into dsa_slave_setup_tagger().
>>
>> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>> ---
>>   net/dsa/slave.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 22ce11cd770e..ae2a648ed9be 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -1808,6 +1808,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>>   	struct dsa_slave_priv *p = netdev_priv(slave);
>>   	const struct dsa_port *cpu_dp = dp->cpu_dp;
>>   	struct net_device *master = cpu_dp->master;
>> +	const struct dsa_switch *ds = dp->ds;
>>   
>>   	slave->needed_headroom = cpu_dp->tag_ops->needed_headroom;
>>   	slave->needed_tailroom = cpu_dp->tag_ops->needed_tailroom;
>> @@ -1819,6 +1820,14 @@ void dsa_slave_setup_tagger(struct net_device *slave)
>>   	slave->needed_tailroom += master->needed_tailroom;
>>   
>>   	p->xmit = cpu_dp->tag_ops->xmit;
>> +
>> +	slave->features = master->vlan_features | NETIF_F_HW_TC;
>> +	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> +		slave->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> +	slave->hw_features |= NETIF_F_HW_TC;
>> +	slave->features |= NETIF_F_LLTX;
>> +	if (slave->needed_tailroom)
>> +		slave->features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
>>   }
>>   
>>   static struct lock_class_key dsa_slave_netdev_xmit_lock_key;
>> @@ -1881,11 +1890,6 @@ int dsa_slave_create(struct dsa_port *port)
>>   	if (slave_dev == NULL)
>>   		return -ENOMEM;
>>   
>> -	slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>> -	if (ds->ops->port_vlan_add && ds->ops->port_vlan_del)
>> -		slave_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> -	slave_dev->hw_features |= NETIF_F_HW_TC;
>> -	slave_dev->features |= NETIF_F_LLTX;
>>   	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>>   	if (!is_zero_ether_addr(port->mac))
>>   		ether_addr_copy(slave_dev->dev_addr, port->mac);
>> -- 
>> 2.32.0
>>
> 
> I would have probably changed the code in dsa_slave_create just like
> this:
> 
> -	slave->features = master->vlan_features | NETIF_F_HW_TC;
> +	slave->features = NETIF_F_HW_TC;
> ...
> -	slave_dev->vlan_features = master->vlan_features;
> 
> and in dsa_slave_setup_tagger:
> 
> +	vlan_features = master->vlan_features;
> +	slave->features &= ~vlan_features;
> +	if (slave->needed_tailroom)
> +		vlan_features &= ~(NETIF_F_SG | NETIF_F_FRAGLIST);
> +	slave->features |= vlan_features;
> +	slave->vlan_features = vlan_features;
> 
> no need to move around NETIF_F_HW_TC and NETIF_F_LLTX. Makes sense?
> 
> And I would probably add:
> 
> Fixes: 91da11f870f0 ("net: Distributed Switch Architecture protocol support")

Agreed, with those fixed:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Andrew Lunn]

> Agreed, with those fixed:
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Hi Florian, Vladimir

I would suggest stop adding Reviewed-by: when you actual want changes
made. The bot does not seem to be reading the actual emails, it just
looks for tags. And when there are sufficient tags, it merges,
independent of requests for change, open questions, etc.

	    Andrew

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Florian Fainelli]

On 7/22/21 7:14 AM, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Hi Florian, Vladimir
> 
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.

Yes, I will definitively stop doing that. I did not think that the
merging was handled by a bot, but if it is, that makes me seriously
nervous about the whole process.
-- 
Florian

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Lino Sanfilippo]

On 22.07.21 at 16:14, Andrew Lunn wrote:
>> Agreed, with those fixed:
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Hi Florian, Vladimir
>
> I would suggest stop adding Reviewed-by: when you actual want changes
> made. The bot does not seem to be reading the actual emails, it just
> looks for tags. And when there are sufficient tags, it merges,
> independent of requests for change, open questions, etc.
>
> 	    Andrew
>

Hi,

since I got a message that the patches have already been applied to netdev/net.git.
How should I proceed if I want to send a new version of the series? Just ignore the
merge to netdev and send the patches nevertheless?

Regards,
Lino

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Vladimir Oltean]

On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
> since I got a message that the patches have already been applied to netdev/net.git.
> How should I proceed if I want to send a new version of the series? Just ignore the
> merge to netdev and send the patches nevertheless?

Since the git history is immutable you need to work with what is already
in the current net/master branch. What do you want to change, just
address the feedback I gave? If that is all, just don't bother, I intend
to look at adding a framework through which the DSA master can declare
what features it supports in conjunction with specific DSA tagging protocols.
That is material for net-next, and Dave took your patch at the last
minute for the "net" pull request towards Linus' tree. If you send
another patch on "net" in that area now, we'd have to wait for another
week or two until "net" will be merged again into "net-next". Not sure
if it's worth it. The only thing that was of concern to me is that you
assign the DSA interface's slave->vlan_features = master->vlan_features.
So even though you clear the NETIF_F_SG feature for the DSA slave
interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
However, those skbs will be linearized during the dev_queue_xmit call
done by the 8021q driver towards DSA, so in the end, the way in which
you restructured the code may not be cosmetically ideal, but also
appears to not be functionally problematic.
Anyway, your patch will probably conflict with the stable trees (the
tag_ops->needed_tailroom was introduced very recently), so we will have
another chance to fix it up when Greg sends the email that the patch
failed to apply.

Re: [PATCH v2 1/2] net: dsa: ensure linearized SKBs in case of tail taggers

[Lino Sanfilippo]

Hi Vladimir,

On 23.07.21 at 14:22, Vladimir Oltean wrote:
> On Fri, Jul 23, 2021 at 09:47:39AM +0200, Lino Sanfilippo wrote:
>> since I got a message that the patches have already been applied to netdev/net.git.
>> How should I proceed if I want to send a new version of the series? Just ignore the
>> merge to netdev and send the patches nevertheless?
>
> Since the git history is immutable you need to work with what is already
> in the current net/master branch. What do you want to change, just
> address the feedback I gave? If that is all, just don't bother, I intend
> to look at adding a framework through which the DSA master can declare
> what features it supports in conjunction with specific DSA tagging protocols.
> That is material for net-next, and Dave took your patch at the last
> minute for the "net" pull request towards Linus' tree. If you send
> another patch on "net" in that area now, we'd have to wait for another
> week or two until "net" will be merged again into "net-next". Not sure
> if it's worth it. The only thing that was of concern to me is that you
> assign the DSA interface's slave->vlan_features = master->vlan_features.
> So even though you clear the NETIF_F_SG feature for the DSA slave
> interface, VLAN uppers on top of DSA interfaces will still have NETIF_F_SG.
> However, those skbs will be linearized during the dev_queue_xmit call
> done by the 8021q driver towards DSA, so in the end, the way in which
> you restructured the code may not be cosmetically ideal, but also
> appears to not be functionally problematic.
> Anyway, your patch will probably conflict with the stable trees (the
> tag_ops->needed_tailroom was introduced very recently), so we will have
> another chance to fix it up when Greg sends the email that the patch
> failed to apply.
>

Yes, I just wanted to address your feedback concerning the feature assignment
in a new patch version. But as you explained this is not needed and would make
things just unnecessary complicated. So lets wait and see if there are any
conflicts with Gregs stable tree. Thanks for the explanation.

Best Regards,
Lino

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

[Lino Sanfilippo]

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 considered as being part of the daa 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>
---
 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 53565f48934c..a201ccf2435d 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -53,6 +53,9 @@ static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 *tag;
 	u8 *addr;
 
+	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);
@@ -114,6 +117,9 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 	u8 *addr;
 	u16 val;
 
+	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);
@@ -164,6 +170,9 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	u8 *addr;
 	u8 *tag;
 
+	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

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

[Vladimir Oltean]

On Wed, Jul 21, 2021 at 11:56:42PM +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 considered as being part of the daa 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>
> ---

Fixes: 8b8010fb7876 ("dsa: add support for Microchip KSZ tail tagging")
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[Florian Fainelli]

On 7/21/2021 2:56 PM, 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 considered as being part of the daa 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2 0/2] Fixes for KSZ DSA switch

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Wed, 21 Jul 2021 23:56:40 +0200 you 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.
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] net: dsa: ensure linearized SKBs in case of tail taggers
    https://git.kernel.org/netdev/net/c/21cf377a9c40
  - [v2,2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum
    https://git.kernel.org/netdev/net/c/37120f23ac89

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html