[PATCH net-next] net/mlx5e: Don't allow forwarding between uplink

From: Tonghao Zhang <redacted>

We can install forwarding packets rule between uplink
in eswitchdev mode, as show below. But the hardware does
not do that as expected (mlnx_perf -i $PF1, we can't get
the counter of the PF1). By the way, if we add the uplink
PF0, PF1 to Open vSwitch and enable hw-offload, the rules
can be offloaded but not work fine too. This patch add a
check and if so return -EOPNOTSUPP.

$ tc filter add dev $PF0 protocol all parent ffff: prio 1 handle 1 \
    flower skip_sw action mirred egress redirect dev $PF1

$ tc -d -s filter show dev $PF0 ingress
    skip_sw
    in_hw in_hw_count 1
    action order 1: mirred (Egress Redirect to device enp130s0f1) stolen
        ...
	Sent 408954 bytes 4173 pkt (dropped 0, overlimits 0 requeues 0)
	Sent hardware 408954 bytes 4173 pkt
	...

Signed-off-by: Tonghao Zhang <redacted>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c |  7 ++++++-
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c  | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index f175cb2..63fad66 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

@@ -1434,10 +1434,15 @@ static struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev)
 	.ndo_set_features        = mlx5e_set_features,
 };
 
+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev)
+{
+	return netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep;
+}
+
 bool mlx5e_eswitch_rep(struct net_device *netdev)
 {
 	if (netdev->netdev_ops == &mlx5e_netdev_ops_rep ||
-	    netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep)
+	    mlx5e_eswitch_uplink_rep(netdev))
 		return true;
 
 	return false;

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index 31f83c8..282c64b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h

@@ -198,6 +198,7 @@ void mlx5e_rep_encap_entry_detach(struct mlx5e_priv *priv,
 
 void mlx5e_rep_queue_neigh_stats_work(struct mlx5e_priv *priv);
 
+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev);
 bool mlx5e_eswitch_rep(struct net_device *netdev);
 
 #else /* CONFIG_MLX5_ESWITCH */

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index f91e057e..b2c18fa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

@@ -3214,6 +3214,10 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
 bool mlx5e_is_valid_eswitch_fwd_dev(struct mlx5e_priv *priv,
 				    struct net_device *out_dev)
 {
+	if (mlx5e_eswitch_uplink_rep(priv->netdev) &&
+	    mlx5e_eswitch_uplink_rep(out_dev))
+		return false;
+
 	if (is_merged_eswitch_dev(priv, out_dev))
 		return true;
 

@@ -3339,9 +3343,15 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 
 				if (!mlx5e_is_valid_eswitch_fwd_dev(priv, out_dev)) {
 					NL_SET_ERR_MSG_MOD(extack,
-							   "devices are not on same switch HW, can't offload forwarding");
-					pr_err("devices %s %s not on same switch HW, can't offload forwarding\n",
-					       priv->netdev->name, out_dev->name);
+							   "devices are both uplink "
+							   "or not on same switch HW, "
+							   "can't offload forwarding");
+					pr_err("devices %s %s are both uplink "
+					       "or not on same switch HW, "
+					       "can't offload forwarding\n",
+					       priv->netdev->name,
+					       out_dev->name);
+
 					return -EOPNOTSUPP;
 				}
 

-- 
1.8.3.1

On Tue, Jan 7, 2020 at 11:52 AM [off-list ref] wrote:

We can install forwarding packets rule between uplink
in eswitchdev mode, as show below. But the hardware does
not do that as expected (mlnx_perf -i $PF1, we can't get
the counter of the PF1). By the way, if we add the uplink

yeah, IMOH there's probably a firmware bug under which installation
of this rule doesn't fail but as you said the traffic doesn't pass.

We don't fail the rule b/c due to what we call "merged eswitch"
which is a building block of the uplink LAG and  ECMP configurations
that we do support, the driver lets you install a flow from (say) VF0
on PF/uplink0 to uplink1 etc.

Talking to Roi, he prefers to further investigate / open a case to the FW
and later see if/how to block this rule in the driver.

On Thu, Jan 9, 2020 at 6:32 PM Or Gerlitz [off-list ref] wrote:

On Tue, Jan 7, 2020 at 11:52 AM [off-list ref] wrote:

quoted

We can install forwarding packets rule between uplink
in eswitchdev mode, as show below. But the hardware does
not do that as expected (mlnx_perf -i $PF1, we can't get
the counter of the PF1). By the way, if we add the uplink

yeah, IMOH there's probably a firmware bug under which installation
of this rule doesn't fail but as you said the traffic doesn't pass.

We don't fail the rule b/c due to what we call "merged eswitch"
which is a building block of the uplink LAG and  ECMP configurations
that we do support, the driver lets you install a flow from (say) VF0
on PF/uplink0 to uplink1 etc.

Talking to Roi, he prefers to further investigate / open a case to the FW
and later see if/how to block this rule in the driver.

make any process, please let me know, thanks

quoted

PF0, PF1 to Open vSwitch and enable hw-offload, the rules
can be offloaded but not work fine too. This patch add a
check and if so return -EOPNOTSUPP.

$ tc filter add dev $PF0 protocol all parent ffff: prio 1 handle 1 \
    flower skip_sw action mirred egress redirect dev $PF1

$ tc -d -s filter show dev $PF0 ingress
    skip_sw
    in_hw in_hw_count 1
    action order 1: mirred (Egress Redirect to device enp130s0f1) stolen
        ...
        Sent 408954 bytes 4173 pkt (dropped 0, overlimits 0 requeues 0)
        Sent hardware 408954 bytes 4173 pkt
        ...

Signed-off-by: Tonghao Zhang <redacted>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c |  7 ++++++-
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c  | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index f175cb2..63fad66 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

@@ -1434,10 +1434,15 @@ static struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev)
        .ndo_set_features        = mlx5e_set_features,
 };

+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev)
+{
+       return netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep;
+}
+
 bool mlx5e_eswitch_rep(struct net_device *netdev)
 {
        if (netdev->netdev_ops == &mlx5e_netdev_ops_rep ||
-           netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep)
+           mlx5e_eswitch_uplink_rep(netdev))
                return true;

        return false;

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index 31f83c8..282c64b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h

@@ -198,6 +198,7 @@ void mlx5e_rep_encap_entry_detach(struct mlx5e_priv *priv,

 void mlx5e_rep_queue_neigh_stats_work(struct mlx5e_priv *priv);

+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev);
 bool mlx5e_eswitch_rep(struct net_device *netdev);

 #else /* CONFIG_MLX5_ESWITCH */

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index f91e057e..b2c18fa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

@@ -3214,6 +3214,10 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
 bool mlx5e_is_valid_eswitch_fwd_dev(struct mlx5e_priv *priv,
                                    struct net_device *out_dev)
 {
+       if (mlx5e_eswitch_uplink_rep(priv->netdev) &&
+           mlx5e_eswitch_uplink_rep(out_dev))
+               return false;
+
        if (is_merged_eswitch_dev(priv, out_dev))
                return true;

@@ -3339,9 +3343,15 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,

                                if (!mlx5e_is_valid_eswitch_fwd_dev(priv, out_dev)) {
                                        NL_SET_ERR_MSG_MOD(extack,
-                                                          "devices are not on same switch HW, can't offload forwarding");
-                                       pr_err("devices %s %s not on same switch HW, can't offload forwarding\n",
-                                              priv->netdev->name, out_dev->name);
+                                                          "devices are both uplink "
+                                                          "or not on same switch HW, "
+                                                          "can't offload forwarding");
+                                       pr_err("devices %s %s are both uplink "
+                                              "or not on same switch HW, "
+                                              "can't offload forwarding\n",
+                                              priv->netdev->name,
+                                              out_dev->name);
+
                                        return -EOPNOTSUPP;
                                }

--

1.8.3.1

On 2020-01-09 1:05 PM, Tonghao Zhang wrote:

On Thu, Jan 9, 2020 at 6:32 PM Or Gerlitz [off-list ref] wrote:

quoted

On Tue, Jan 7, 2020 at 11:52 AM [off-list ref] wrote:

quoted

We can install forwarding packets rule between uplink
in eswitchdev mode, as show below. But the hardware does
not do that as expected (mlnx_perf -i $PF1, we can't get
the counter of the PF1). By the way, if we add the uplink

yeah, IMOH there's probably a firmware bug under which installation
of this rule doesn't fail but as you said the traffic doesn't pass.

We don't fail the rule b/c due to what we call "merged eswitch"
which is a building block of the uplink LAG and  ECMP configurations
that we do support, the driver lets you install a flow from (say) VF0
on PF/uplink0 to uplink1 etc.

Talking to Roi, he prefers to further investigate / open a case to the FW
and later see if/how to block this rule in the driver.

make any process, please let me know, thanks

quoted

quoted

PF0, PF1 to Open vSwitch and enable hw-offload, the rules
can be offloaded but not work fine too. This patch add a
check and if so return -EOPNOTSUPP.

$ tc filter add dev $PF0 protocol all parent ffff: prio 1 handle 1 \
    flower skip_sw action mirred egress redirect dev $PF1

$ tc -d -s filter show dev $PF0 ingress
    skip_sw
    in_hw in_hw_count 1
    action order 1: mirred (Egress Redirect to device enp130s0f1) stolen
        ...
        Sent 408954 bytes 4173 pkt (dropped 0, overlimits 0 requeues 0)
        Sent hardware 408954 bytes 4173 pkt
        ...

Signed-off-by: Tonghao Zhang <redacted>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c |  7 ++++++-
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c  | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index f175cb2..63fad66 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

@@ -1434,10 +1434,15 @@ static struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev)
        .ndo_set_features        = mlx5e_set_features,
 };

+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev)
+{
+       return netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep;
+}
+
 bool mlx5e_eswitch_rep(struct net_device *netdev)
 {
        if (netdev->netdev_ops == &mlx5e_netdev_ops_rep ||
-           netdev->netdev_ops == &mlx5e_netdev_ops_uplink_rep)
+           mlx5e_eswitch_uplink_rep(netdev))
                return true;

        return false;

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index 31f83c8..282c64b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h

@@ -198,6 +198,7 @@ void mlx5e_rep_encap_entry_detach(struct mlx5e_priv *priv,

 void mlx5e_rep_queue_neigh_stats_work(struct mlx5e_priv *priv);

+bool mlx5e_eswitch_uplink_rep(struct net_device *netdev);
 bool mlx5e_eswitch_rep(struct net_device *netdev);

 #else /* CONFIG_MLX5_ESWITCH */

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index f91e057e..b2c18fa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

@@ -3214,6 +3214,10 @@ static int add_vlan_pop_action(struct mlx5e_priv *priv,
 bool mlx5e_is_valid_eswitch_fwd_dev(struct mlx5e_priv *priv,
                                    struct net_device *out_dev)
 {
+       if (mlx5e_eswitch_uplink_rep(priv->netdev) &&
+           mlx5e_eswitch_uplink_rep(out_dev))
+               return false;
+
        if (is_merged_eswitch_dev(priv, out_dev))
                return true;

@@ -3339,9 +3343,15 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,

                                if (!mlx5e_is_valid_eswitch_fwd_dev(priv, out_dev)) {
                                        NL_SET_ERR_MSG_MOD(extack,
-                                                          "devices are not on same switch HW, can't offload forwarding");
-                                       pr_err("devices %s %s not on same switch HW, can't offload forwarding\n",
-                                              priv->netdev->name, out_dev->name);
+                                                          "devices are both uplink "
+                                                          "or not on same switch HW, "
+                                                          "can't offload forwarding");
+                                       pr_err("devices %s %s are both uplink "
+                                              "or not on same switch HW, "
+                                              "can't offload forwarding\n",
+                                              priv->netdev->name,
+                                              out_dev->name);
+
                                        return -EOPNOTSUPP;
                                }

--

1.8.3.1

Hi,

I forwarded the issue to fw for a response.
Though, we can progress with blocking uplink->uplink rules but this patch
currently breaks LAG mode.
In LAG mode we add a rule to both esw0 of uplink0 and esw1 of uplink1,
so for the check it looks like priv and out_dev are both uplinks,
but this is allowed as Or said. we forward to the other eswitch but
different vport. so the check is missing if the vport is uplink.

so normally priv is the representor priv but in case of LAG

mlx5e_add_fdb_flow()->is_peer_flow_needed()->mlx5e_tc_add_fdb_peer_flow()

we pass priv as the uplink of the other eswitch.
the vport is saved in esw_attr->in_rep.
so for this case we need to compare if in_rep (and not priv) is actually
vf representor or uplink representor.

the case i tested is create bond0 with the 2 uplinks.
add rule from representor to bond0.
e.g. tc filter add dev ens1f0_0 protocol arp parent ffff: prio 3 flower \
  dst_mac e4:1d:2d:fd:8b:02 skip_sw action mirred egress redirect dev bond0


Thanks,
Roi