[PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

By default, the VCAP IS2 will produce a single match for each frame, on
the most specific classification.

Example: a ping packet (ICMP over IPv4 over Ethernet) sent from an IP
address of 10.0.0.1 and a MAC address of 96:18:82:00:04:01 will match
this rule:

tc filter add dev swp0 ingress protocol ipv4 \
	flower skip_sw src_ip 10.0.0.1 action drop

but not this one:

tc filter add dev swp0 ingress \
	flower skip_sw src_mac 96:18:82:00:04:01 action drop

Currently the driver does not really warn the user in any way about
this, and the behavior is rather strange anyway.

The current patch is a workaround to force matches on MAC_ETYPE keys
(DMAC and SMAC) for all packets irrespective of higher layer protocol.
The setting is made at the port level.

Of course this breaks all other non-src_mac and non-dst_mac matches, so
rule exclusivity checks have been added to the driver, in order to never
have rules of both types on any ingress port.

The bits that discard higher-level protocol information are set only
once a MAC_ETYPE rule is added to a filter block, and only for the ports
that are bound to that filter block. Then all further non-MAC_ETYPE
rules added to that filter block should be denied by the ports bound to
it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_ace.c    | 103 +++++++++++++++++++++-
 drivers/net/ethernet/mscc/ocelot_ace.h    |   5 +-
 drivers/net/ethernet/mscc/ocelot_flower.c |   2 +-
 3 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
index 3bd286044480..8a2f7d13ef6d 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.c
+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
@@ -706,13 +706,114 @@ ocelot_ace_rule_get_rule_index(struct ocelot_acl_block *block, int index)
 	return NULL;
 }
 
+/* If @on=false, then SNAP, ARP, IP and OAM frames will not match on keys based
+ * on destination and source MAC addresses, but only on higher-level protocol
+ * information. The only frame types to match on keys containing MAC addresses
+ * in this case are non-SNAP, non-ARP, non-IP and non-OAM frames.
+ *
+ * If @on=true, then the above frame types (SNAP, ARP, IP and OAM) will match
+ * on MAC_ETYPE keys such as destination and source MAC on this ingress port.
+ * However the setting has the side effect of making these frames not matching
+ * on any _other_ keys than MAC_ETYPE ones.
+ */
+static void ocelot_match_all_as_mac_etype(struct ocelot *ocelot, int port,
+					  bool on)
+{
+	u32 val = 0;
+
+	if (on)
+		val = ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS(3) |
+		      ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS(3) |
+		      ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS(3) |
+		      ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS(3) |
+		      ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS(3);
+
+	ocelot_rmw_gix(ocelot, val,
+		       ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS_M |
+		       ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS_M |
+		       ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS_M |
+		       ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS_M |
+		       ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS_M,
+		       ANA_PORT_VCAP_S2_CFG, port);
+}
+
+static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
+{
+	if (ace->type != OCELOT_ACE_TYPE_ETYPE)
+		return false;
+	if (ether_addr_to_u64(ace->frame.etype.dmac.value) &
+	    ether_addr_to_u64(ace->frame.etype.dmac.mask))
+		return true;
+	if (ether_addr_to_u64(ace->frame.etype.smac.value) &
+	    ether_addr_to_u64(ace->frame.etype.smac.mask))
+		return true;
+	return false;
+}
+
+static bool ocelot_ace_is_problematic_non_mac_etype(struct ocelot_ace_rule *ace)
+{
+	if (ace->type == OCELOT_ACE_TYPE_SNAP)
+		return true;
+	if (ace->type == OCELOT_ACE_TYPE_ARP)
+		return true;
+	if (ace->type == OCELOT_ACE_TYPE_IPV4)
+		return true;
+	if (ace->type == OCELOT_ACE_TYPE_IPV6)
+		return true;
+	return false;
+}
+
+static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot,
+						 struct ocelot_ace_rule *ace)
+{
+	struct ocelot_acl_block *block = &ocelot->acl_block;
+	struct ocelot_ace_rule *tmp;
+	unsigned long port;
+	int i;
+
+	if (ocelot_ace_is_problematic_mac_etype(ace)) {
+		/* Search for any non-MAC_ETYPE rules on the port */
+		for (i = 0; i < block->count; i++) {
+			tmp = ocelot_ace_rule_get_rule_index(block, i);
+			if (tmp->ingress_port_mask & ace->ingress_port_mask &&
+			    ocelot_ace_is_problematic_non_mac_etype(tmp))
+				return false;
+		}
+
+		for_each_set_bit(port, &ace->ingress_port_mask,
+				 ocelot->num_phys_ports)
+			ocelot_match_all_as_mac_etype(ocelot, port, true);
+	} else if (ocelot_ace_is_problematic_non_mac_etype(ace)) {
+		/* Search for any MAC_ETYPE rules on the port */
+		for (i = 0; i < block->count; i++) {
+			tmp = ocelot_ace_rule_get_rule_index(block, i);
+			if (tmp->ingress_port_mask & ace->ingress_port_mask &&
+			    ocelot_ace_is_problematic_mac_etype(tmp))
+				return false;
+		}
+
+		for_each_set_bit(port, &ace->ingress_port_mask,
+				 ocelot->num_phys_ports)
+			ocelot_match_all_as_mac_etype(ocelot, port, false);
+	}
+
+	return true;
+}
+
 int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
-				struct ocelot_ace_rule *rule)
+				struct ocelot_ace_rule *rule,
+				struct netlink_ext_ack *extack)
 {
 	struct ocelot_acl_block *block = &ocelot->acl_block;
 	struct ocelot_ace_rule *ace;
 	int i, index;
 
+	if (!ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Cannot mix MAC_ETYPE with non-MAC_ETYPE rules");
+		return -EBUSY;
+	}
+
 	/* Add rule to the linked list */
 	ocelot_ace_rule_add(ocelot, block, rule);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h
index 29d22c566786..099e177f2617 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.h
+++ b/drivers/net/ethernet/mscc/ocelot_ace.h
@@ -194,7 +194,7 @@ struct ocelot_ace_rule {
 
 	enum ocelot_ace_action action;
 	struct ocelot_ace_stats stats;
-	u16 ingress_port_mask;
+	unsigned long ingress_port_mask;
 
 	enum ocelot_vcap_bit dmac_mc;
 	enum ocelot_vcap_bit dmac_bc;
@@ -215,7 +215,8 @@ struct ocelot_ace_rule {
 };
 
 int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
-				struct ocelot_ace_rule *rule);
+				struct ocelot_ace_rule *rule,
+				struct netlink_ext_ack *extack);
 int ocelot_ace_rule_offload_del(struct ocelot *ocelot,
 				struct ocelot_ace_rule *rule);
 int ocelot_ace_rule_stats_update(struct ocelot *ocelot,
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 341923311fec..954cb67eeaa2 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -205,7 +205,7 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port,
 		return ret;
 	}
 
-	return ocelot_ace_rule_offload_add(ocelot, ace);
+	return ocelot_ace_rule_offload_add(ocelot, ace, f->common.extack);
 }
 EXPORT_SYMBOL_GPL(ocelot_cls_flower_replace);
 
-- 
2.17.1

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Fri, 17 Apr 2020 22:03:08 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> By default, the VCAP IS2 will produce a single match for each frame, on
> the most specific classification.
> 
> Example: a ping packet (ICMP over IPv4 over Ethernet) sent from an IP
> address of 10.0.0.1 and a MAC address of 96:18:82:00:04:01 will match
> this rule:
> 
> tc filter add dev swp0 ingress protocol ipv4 \
> 	flower skip_sw src_ip 10.0.0.1 action drop
> 
> but not this one:
> 
> tc filter add dev swp0 ingress \
> 	flower skip_sw src_mac 96:18:82:00:04:01 action drop
> 
> Currently the driver does not really warn the user in any way about
> this, and the behavior is rather strange anyway.
> 
> The current patch is a workaround to force matches on MAC_ETYPE keys
> (DMAC and SMAC) for all packets irrespective of higher layer protocol.
> The setting is made at the port level.
> 
> Of course this breaks all other non-src_mac and non-dst_mac matches, so
> rule exclusivity checks have been added to the driver, in order to never
> have rules of both types on any ingress port.
> 
> The bits that discard higher-level protocol information are set only
> once a MAC_ETYPE rule is added to a filter block, and only for the ports
> that are bound to that filter block. Then all further non-MAC_ETYPE
> rules added to that filter block should be denied by the ports bound to
> it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied.

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Allan W. Nielsen]

Hi,

Sorry I did not manage to provide feedback before it was merged (I will
need to consult some of my colleagues Monday before I can provide the
foll feedback).

There are many good things in this patch, but it is not only good.

The problem is that these TCAMs/VCAPs are insanely complicated and it is
really hard to make them fit nicely into the existing tc frame-work
(being hard does not mean that we should not try).

In this patch, you try to automatic figure out who the user want the
TCAM to be configured. It works for 1 use-case but it breaks others.

Before this patch you could do a:
     tc filter add dev swp0 ingress protocol ipv4 \
             flower skip_sw src_ip 10.0.0.1 action drop
     tc filter add dev swp0 ingress \
             flower skip_sw src_mac 96:18:82:00:04:01 action drop

But the second rule would not apply to the ICMP over IPv4 over Ethernet
packet, it would however apply to non-IP packets.

With this patch it not possible. Your use-case is more common, but the
other one is not unrealistic.

My concern with this, is that I do not think it is possible to automatic
detect how these TCAMs needs to be configured by only looking at the
rules installed by the user. Trying to do this automatic, also makes the
TCAM logic even harder to understand for the user.

I would prefer that we by default uses some conservative default
settings which are easy to understand, and then expose some expert
settings in the sysfs, which can be used to achieve different
behavioral.

Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
understand default.

But I do seem to recall that there is a way to allow matching on both
SMAC and SIP (your original motivation). This may be a better default
(despite that it consumes more TCAM resources). I will follow up and
check if this is possible.

Vladimir (and anyone else whom interested): would you be interested in
spending some time discussion the more high-level architectures and
use-cases on how to best integrate this TCAM architecture into the Linux
kernel. Not sure on the outlook for the various conferences, but we
could arrange some online session to discuss this.

/Allan


On 17.04.2020 22:03, Vladimir Oltean wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>By default, the VCAP IS2 will produce a single match for each frame, on
>the most specific classification.
>
>Example: a ping packet (ICMP over IPv4 over Ethernet) sent from an IP
>address of 10.0.0.1 and a MAC address of 96:18:82:00:04:01 will match
>this rule:
>
>tc filter add dev swp0 ingress protocol ipv4 \
>        flower skip_sw src_ip 10.0.0.1 action drop
>
>but not this one:
>
>tc filter add dev swp0 ingress \
>        flower skip_sw src_mac 96:18:82:00:04:01 action drop
>
>Currently the driver does not really warn the user in any way about
>this, and the behavior is rather strange anyway.
>
>The current patch is a workaround to force matches on MAC_ETYPE keys
>(DMAC and SMAC) for all packets irrespective of higher layer protocol.
>The setting is made at the port level.
>
>Of course this breaks all other non-src_mac and non-dst_mac matches, so
>rule exclusivity checks have been added to the driver, in order to never
>have rules of both types on any ingress port.
>
>The bits that discard higher-level protocol information are set only
>once a MAC_ETYPE rule is added to a filter block, and only for the ports
>that are bound to that filter block. Then all further non-MAC_ETYPE
>rules added to that filter block should be denied by the ports bound to
>it.
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
> drivers/net/ethernet/mscc/ocelot_ace.c    | 103 +++++++++++++++++++++-
> drivers/net/ethernet/mscc/ocelot_ace.h    |   5 +-
> drivers/net/ethernet/mscc/ocelot_flower.c |   2 +-
> 3 files changed, 106 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
>index 3bd286044480..8a2f7d13ef6d 100644
>--- a/drivers/net/ethernet/mscc/ocelot_ace.c
>+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
>@@ -706,13 +706,114 @@ ocelot_ace_rule_get_rule_index(struct ocelot_acl_block *block, int index)
>        return NULL;
> }
>
>+/* If @on=false, then SNAP, ARP, IP and OAM frames will not match on keys based
>+ * on destination and source MAC addresses, but only on higher-level protocol
>+ * information. The only frame types to match on keys containing MAC addresses
>+ * in this case are non-SNAP, non-ARP, non-IP and non-OAM frames.
>+ *
>+ * If @on=true, then the above frame types (SNAP, ARP, IP and OAM) will match
>+ * on MAC_ETYPE keys such as destination and source MAC on this ingress port.
>+ * However the setting has the side effect of making these frames not matching
>+ * on any _other_ keys than MAC_ETYPE ones.
>+ */
>+static void ocelot_match_all_as_mac_etype(struct ocelot *ocelot, int port,
>+                                         bool on)
>+{
>+       u32 val = 0;
>+
>+       if (on)
>+               val = ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS(3) |
>+                     ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS(3) |
>+                     ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS(3) |
>+                     ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS(3) |
>+                     ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS(3);
>+
>+       ocelot_rmw_gix(ocelot, val,
>+                      ANA_PORT_VCAP_S2_CFG_S2_SNAP_DIS_M |
>+                      ANA_PORT_VCAP_S2_CFG_S2_ARP_DIS_M |
>+                      ANA_PORT_VCAP_S2_CFG_S2_IP_TCPUDP_DIS_M |
>+                      ANA_PORT_VCAP_S2_CFG_S2_IP_OTHER_DIS_M |
>+                      ANA_PORT_VCAP_S2_CFG_S2_OAM_DIS_M,
>+                      ANA_PORT_VCAP_S2_CFG, port);
>+}
>+
>+static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
>+{
>+       if (ace->type != OCELOT_ACE_TYPE_ETYPE)
>+               return false;
>+       if (ether_addr_to_u64(ace->frame.etype.dmac.value) &
>+           ether_addr_to_u64(ace->frame.etype.dmac.mask))
>+               return true;
>+       if (ether_addr_to_u64(ace->frame.etype.smac.value) &
>+           ether_addr_to_u64(ace->frame.etype.smac.mask))
>+               return true;
>+       return false;
>+}
>+
>+static bool ocelot_ace_is_problematic_non_mac_etype(struct ocelot_ace_rule *ace)
>+{
>+       if (ace->type == OCELOT_ACE_TYPE_SNAP)
>+               return true;
>+       if (ace->type == OCELOT_ACE_TYPE_ARP)
>+               return true;
>+       if (ace->type == OCELOT_ACE_TYPE_IPV4)
>+               return true;
>+       if (ace->type == OCELOT_ACE_TYPE_IPV6)
>+               return true;
>+       return false;
>+}
>+
>+static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot,
>+                                                struct ocelot_ace_rule *ace)
>+{
>+       struct ocelot_acl_block *block = &ocelot->acl_block;
>+       struct ocelot_ace_rule *tmp;
>+       unsigned long port;
>+       int i;
>+
>+       if (ocelot_ace_is_problematic_mac_etype(ace)) {
>+               /* Search for any non-MAC_ETYPE rules on the port */
>+               for (i = 0; i < block->count; i++) {
>+                       tmp = ocelot_ace_rule_get_rule_index(block, i);
>+                       if (tmp->ingress_port_mask & ace->ingress_port_mask &&
>+                           ocelot_ace_is_problematic_non_mac_etype(tmp))
>+                               return false;
>+               }
>+
>+               for_each_set_bit(port, &ace->ingress_port_mask,
>+                                ocelot->num_phys_ports)
>+                       ocelot_match_all_as_mac_etype(ocelot, port, true);
>+       } else if (ocelot_ace_is_problematic_non_mac_etype(ace)) {
>+               /* Search for any MAC_ETYPE rules on the port */
>+               for (i = 0; i < block->count; i++) {
>+                       tmp = ocelot_ace_rule_get_rule_index(block, i);
>+                       if (tmp->ingress_port_mask & ace->ingress_port_mask &&
>+                           ocelot_ace_is_problematic_mac_etype(tmp))
>+                               return false;
>+               }
>+
>+               for_each_set_bit(port, &ace->ingress_port_mask,
>+                                ocelot->num_phys_ports)
>+                       ocelot_match_all_as_mac_etype(ocelot, port, false);
>+       }
>+
>+       return true;
>+}
>+
> int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
>-                               struct ocelot_ace_rule *rule)
>+                               struct ocelot_ace_rule *rule,
>+                               struct netlink_ext_ack *extack)
> {
>        struct ocelot_acl_block *block = &ocelot->acl_block;
>        struct ocelot_ace_rule *ace;
>        int i, index;
>
>+       if (!ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) {
>+               NL_SET_ERR_MSG_MOD(extack,
>+                                  "Cannot mix MAC_ETYPE with non-MAC_ETYPE rules");
>+               return -EBUSY;
>+       }
>+
>        /* Add rule to the linked list */
>        ocelot_ace_rule_add(ocelot, block, rule);
>
>diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h
>index 29d22c566786..099e177f2617 100644
>--- a/drivers/net/ethernet/mscc/ocelot_ace.h
>+++ b/drivers/net/ethernet/mscc/ocelot_ace.h
>@@ -194,7 +194,7 @@ struct ocelot_ace_rule {
>
>        enum ocelot_ace_action action;
>        struct ocelot_ace_stats stats;
>-       u16 ingress_port_mask;
>+       unsigned long ingress_port_mask;
>
>        enum ocelot_vcap_bit dmac_mc;
>        enum ocelot_vcap_bit dmac_bc;
>@@ -215,7 +215,8 @@ struct ocelot_ace_rule {
> };
>
> int ocelot_ace_rule_offload_add(struct ocelot *ocelot,
>-                               struct ocelot_ace_rule *rule);
>+                               struct ocelot_ace_rule *rule,
>+                               struct netlink_ext_ack *extack);
> int ocelot_ace_rule_offload_del(struct ocelot *ocelot,
>                                struct ocelot_ace_rule *rule);
> int ocelot_ace_rule_stats_update(struct ocelot *ocelot,
>diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
>index 341923311fec..954cb67eeaa2 100644
>--- a/drivers/net/ethernet/mscc/ocelot_flower.c
>+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
>@@ -205,7 +205,7 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port,
>                return ret;
>        }
>
>-       return ocelot_ace_rule_offload_add(ocelot, ace);
>+       return ocelot_ace_rule_offload_add(ocelot, ace, f->common.extack);
> }
> EXPORT_SYMBOL_GPL(ocelot_cls_flower_replace);
>
>--
>2.17.1
>
/Allan

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Ido Schimmel]

On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
> Hi,
> 
> Sorry I did not manage to provide feedback before it was merged (I will
> need to consult some of my colleagues Monday before I can provide the
> foll feedback).
> 
> There are many good things in this patch, but it is not only good.
> 
> The problem is that these TCAMs/VCAPs are insanely complicated and it is
> really hard to make them fit nicely into the existing tc frame-work
> (being hard does not mean that we should not try).
> 
> In this patch, you try to automatic figure out who the user want the
> TCAM to be configured. It works for 1 use-case but it breaks others.
> 
> Before this patch you could do a:
>     tc filter add dev swp0 ingress protocol ipv4 \
>             flower skip_sw src_ip 10.0.0.1 action drop
>     tc filter add dev swp0 ingress \
>             flower skip_sw src_mac 96:18:82:00:04:01 action drop
> 
> But the second rule would not apply to the ICMP over IPv4 over Ethernet
> packet, it would however apply to non-IP packets.
> 
> With this patch it not possible. Your use-case is more common, but the
> other one is not unrealistic.
> 
> My concern with this, is that I do not think it is possible to automatic
> detect how these TCAMs needs to be configured by only looking at the
> rules installed by the user. Trying to do this automatic, also makes the
> TCAM logic even harder to understand for the user.
> 
> I would prefer that we by default uses some conservative default
> settings which are easy to understand, and then expose some expert
> settings in the sysfs, which can be used to achieve different
> behavioral.
> 
> Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> understand default.
> 
> But I do seem to recall that there is a way to allow matching on both
> SMAC and SIP (your original motivation). This may be a better default
> (despite that it consumes more TCAM resources). I will follow up and
> check if this is possible.
> 
> Vladimir (and anyone else whom interested): would you be interested in
> spending some time discussion the more high-level architectures and
> use-cases on how to best integrate this TCAM architecture into the Linux
> kernel. Not sure on the outlook for the various conferences, but we
> could arrange some online session to discuss this.

Not sure I completely understand the difficulties you are facing, but it
sounds similar to a problem we had in mlxsw. You might want to look into
"chain templates" [1] in order to restrict the keys that can be used
simultaneously.

I don't mind participating in an online discussion if you think it can
help.

[1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Vladimir Oltean]

Hi Ido, Allan,

On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@idosch.org> wrote:
>
> On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
> > Hi,
> >
> > Sorry I did not manage to provide feedback before it was merged (I will
> > need to consult some of my colleagues Monday before I can provide the
> > foll feedback).
> >
> > There are many good things in this patch, but it is not only good.
> >
> > The problem is that these TCAMs/VCAPs are insanely complicated and it is
> > really hard to make them fit nicely into the existing tc frame-work
> > (being hard does not mean that we should not try).
> >
> > In this patch, you try to automatic figure out who the user want the
> > TCAM to be configured. It works for 1 use-case but it breaks others.
> >
> > Before this patch you could do a:
> >     tc filter add dev swp0 ingress protocol ipv4 \
> >             flower skip_sw src_ip 10.0.0.1 action drop
> >     tc filter add dev swp0 ingress \
> >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
> >
> > But the second rule would not apply to the ICMP over IPv4 over Ethernet
> > packet, it would however apply to non-IP packets.
> >
> > With this patch it not possible. Your use-case is more common, but the
> > other one is not unrealistic.
> >
> > My concern with this, is that I do not think it is possible to automatic
> > detect how these TCAMs needs to be configured by only looking at the
> > rules installed by the user. Trying to do this automatic, also makes the
> > TCAM logic even harder to understand for the user.
> >
> > I would prefer that we by default uses some conservative default
> > settings which are easy to understand, and then expose some expert
> > settings in the sysfs, which can be used to achieve different
> > behavioral.
> >
> > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> > understand default.
> >
> > But I do seem to recall that there is a way to allow matching on both
> > SMAC and SIP (your original motivation). This may be a better default
> > (despite that it consumes more TCAM resources). I will follow up and
> > check if this is possible.
> >
> > Vladimir (and anyone else whom interested): would you be interested in
> > spending some time discussion the more high-level architectures and
> > use-cases on how to best integrate this TCAM architecture into the Linux
> > kernel. Not sure on the outlook for the various conferences, but we
> > could arrange some online session to discuss this.
>
> Not sure I completely understand the difficulties you are facing, but it
> sounds similar to a problem we had in mlxsw. You might want to look into
> "chain templates" [1] in order to restrict the keys that can be used
> simultaneously.
>
> I don't mind participating in an online discussion if you think it can
> help.
>
> [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates

I think it is worth giving a bit of context on what motivated me to
add this patch. Luckily I believe I can summarize it in a paragraph
below.

I am trying to understand practical ways in which IEEE 802.1CB can be
used - an active redundancy mechanism similar to HSR/PRP which relies
on sending sequence-numbered frame duplicates and eliminating those
duplicates at the destination (as opposed to passive redundancy
mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
stop L2 forwarding loops from killing the network). So since 802.1CB
needs a network where none of the port states can be put to BLOCKING
(as that would break the forwarding of some of the replicated
streams), I need a way to limit the impact of L2 loops. Currently I am
using, rather successfully, an idea borrowed from HSR called
"self-address filtering". It says that received packets can be dropped
if their source MAC address matches the device's MAC address. This
feature is useful for ensuring that packets never traverse a ring
network more than once.
To implement this idea, I use an offloaded tc-flower rule matching on
src_mac with an action of "drop".

To my surprise, such a src_mac rule does not do what's written on the
box with the Ocelot switch flow classification engine called VCAP IS2.
That is, packets having that src_mac would only get dropped if their
protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
rule is less than useful for the purpose we want it to.
I did raise this concern here, and the suggestion that I got is to
implement something like this patch, aka enable a port setting which
forces matches on MAC_ETYPE keys only, regardless of higher-layer
protocol information:
https://lkml.org/lkml/2020/2/24/489
So the default (pre-patch) behavior is for IP (and other) matches to
be sane, at the expense of MAC matches being insane.
Whereas the current behavior is for MAC matches to be sane, at the
expense of IP matches becoming impossible as long as MAC rules are
also present.
In this context, Allan's complaint seems to be that the MAC matches
were "good enough" for them, even if not all MAC address matches were
caught, at least it did not prevent them from installing IP matching
rules on the same port.

I may not have completely understood Ido's suggestion to use
FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
put to practical use to solve the clash here), but I do believe that
it is only a way for the driver to eliminate the guesswork out of the
user's intention.
In this case, my personal opinion is that the intention is absolutely
clear: a classifier with src_mac should match on all frames having
that src_mac (if that is not commonly agreed here, a good rule of
thumb is to compare with what a non-offloaded tc filter rule does).
Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
to still perform [ without calling ocelot_match_all_as_mac_etype ]
should be expressed in terms of a more specific classification key to
tc, such as:

tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
96:18:82:00:04:01 action drop

In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
then these rules can happily live together without ever needing to
call ocelot_match_all_as_mac_etype. If we agree on this solution, I
can send a patch that refines the ocelot_ace_is_problematic_mac_etype
function.

Thanks,
-Vladimir

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Ido Schimmel]

On Sun, Apr 19, 2020 at 03:47:01PM +0300, Vladimir Oltean wrote:
> Hi Ido, Allan,
> 
> On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@idosch.org> wrote:
> >
> > On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
> > > Hi,
> > >
> > > Sorry I did not manage to provide feedback before it was merged (I will
> > > need to consult some of my colleagues Monday before I can provide the
> > > foll feedback).
> > >
> > > There are many good things in this patch, but it is not only good.
> > >
> > > The problem is that these TCAMs/VCAPs are insanely complicated and it is
> > > really hard to make them fit nicely into the existing tc frame-work
> > > (being hard does not mean that we should not try).
> > >
> > > In this patch, you try to automatic figure out who the user want the
> > > TCAM to be configured. It works for 1 use-case but it breaks others.
> > >
> > > Before this patch you could do a:
> > >     tc filter add dev swp0 ingress protocol ipv4 \
> > >             flower skip_sw src_ip 10.0.0.1 action drop
> > >     tc filter add dev swp0 ingress \
> > >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
> > >
> > > But the second rule would not apply to the ICMP over IPv4 over Ethernet
> > > packet, it would however apply to non-IP packets.
> > >
> > > With this patch it not possible. Your use-case is more common, but the
> > > other one is not unrealistic.
> > >
> > > My concern with this, is that I do not think it is possible to automatic
> > > detect how these TCAMs needs to be configured by only looking at the
> > > rules installed by the user. Trying to do this automatic, also makes the
> > > TCAM logic even harder to understand for the user.
> > >
> > > I would prefer that we by default uses some conservative default
> > > settings which are easy to understand, and then expose some expert
> > > settings in the sysfs, which can be used to achieve different
> > > behavioral.
> > >
> > > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> > > understand default.
> > >
> > > But I do seem to recall that there is a way to allow matching on both
> > > SMAC and SIP (your original motivation). This may be a better default
> > > (despite that it consumes more TCAM resources). I will follow up and
> > > check if this is possible.
> > >
> > > Vladimir (and anyone else whom interested): would you be interested in
> > > spending some time discussion the more high-level architectures and
> > > use-cases on how to best integrate this TCAM architecture into the Linux
> > > kernel. Not sure on the outlook for the various conferences, but we
> > > could arrange some online session to discuss this.
> >
> > Not sure I completely understand the difficulties you are facing, but it
> > sounds similar to a problem we had in mlxsw. You might want to look into
> > "chain templates" [1] in order to restrict the keys that can be used
> > simultaneously.
> >
> > I don't mind participating in an online discussion if you think it can
> > help.
> >
> > [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates
> 
> I think it is worth giving a bit of context on what motivated me to
> add this patch. Luckily I believe I can summarize it in a paragraph
> below.
> 
> I am trying to understand practical ways in which IEEE 802.1CB can be
> used - an active redundancy mechanism similar to HSR/PRP which relies
> on sending sequence-numbered frame duplicates and eliminating those
> duplicates at the destination (as opposed to passive redundancy
> mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
> stop L2 forwarding loops from killing the network). So since 802.1CB
> needs a network where none of the port states can be put to BLOCKING
> (as that would break the forwarding of some of the replicated
> streams), I need a way to limit the impact of L2 loops. Currently I am
> using, rather successfully, an idea borrowed from HSR called
> "self-address filtering". It says that received packets can be dropped
> if their source MAC address matches the device's MAC address. This
> feature is useful for ensuring that packets never traverse a ring
> network more than once.
> To implement this idea, I use an offloaded tc-flower rule matching on
> src_mac with an action of "drop".
> 
> To my surprise, such a src_mac rule does not do what's written on the
> box with the Ocelot switch flow classification engine called VCAP IS2.
> That is, packets having that src_mac would only get dropped if their
> protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
> rule is less than useful for the purpose we want it to.
> I did raise this concern here, and the suggestion that I got is to
> implement something like this patch, aka enable a port setting which
> forces matches on MAC_ETYPE keys only, regardless of higher-layer
> protocol information:
> https://lkml.org/lkml/2020/2/24/489
> So the default (pre-patch) behavior is for IP (and other) matches to
> be sane, at the expense of MAC matches being insane.
> Whereas the current behavior is for MAC matches to be sane, at the
> expense of IP matches becoming impossible as long as MAC rules are
> also present.
> In this context, Allan's complaint seems to be that the MAC matches
> were "good enough" for them, even if not all MAC address matches were
> caught, at least it did not prevent them from installing IP matching
> rules on the same port.
> 
> I may not have completely understood Ido's suggestion to use
> FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
> put to practical use to solve the clash here), but I do believe that
> it is only a way for the driver to eliminate the guesswork out of the
> user's intention.

I was under the impression that you can't mix different keys (e.g.,
src_mac + src_ip), but now I understand that you can't mix different
keys in case of specific key *values*. This will work correctly:

$ tc filter add dev swp0 ingress proto ip \
	flower src_ip 192.0.2.1 action drop
$ tc filter add dev swp0 ingress proto 0x88f7 \
	flower src_mac 00:11:22:33:44:55 action drop

This will not work correctly:

$ tc filter add dev swp0 ingress proto ip \
	flower src_ip 192.0.2.1 action drop
$ tc filter add dev swp0 ingress proto all \
	flower src_mac 00:11:22:33:44:55 action drop

Correct? If so, I don't think the templates can help you. They are about
forcing only specific keys, regardless of value.

> In this case, my personal opinion is that the intention is absolutely
> clear: a classifier with src_mac should match on all frames having
> that src_mac (if that is not commonly agreed here, a good rule of
> thumb is to compare with what a non-offloaded tc filter rule does).

I agree.

> Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
> to still perform [ without calling ocelot_match_all_as_mac_etype ]
> should be expressed in terms of a more specific classification key to
> tc, such as:

Yes.

> 
> tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
> 96:18:82:00:04:01 action drop
> 
> In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
> then these rules can happily live together without ever needing to
> call ocelot_match_all_as_mac_etype. If we agree on this solution, I
> can send a patch that refines the ocelot_ace_is_problematic_mac_etype
> function.

I think it makes sense. You are basically being explicit about the
hardware limitations and denying configurations that cannot always work.
Previous approach was to allow configurations that sometimes work.

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Vladimir Oltean]

On Sun, 19 Apr 2020 at 16:51, Ido Schimmel <idosch@idosch.org> wrote:
>
> On Sun, Apr 19, 2020 at 03:47:01PM +0300, Vladimir Oltean wrote:
> > Hi Ido, Allan,
> >
> > On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@idosch.org> wrote:
> > >
> > > On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
> > > > Hi,
> > > >
> > > > Sorry I did not manage to provide feedback before it was merged (I will
> > > > need to consult some of my colleagues Monday before I can provide the
> > > > foll feedback).
> > > >
> > > > There are many good things in this patch, but it is not only good.
> > > >
> > > > The problem is that these TCAMs/VCAPs are insanely complicated and it is
> > > > really hard to make them fit nicely into the existing tc frame-work
> > > > (being hard does not mean that we should not try).
> > > >
> > > > In this patch, you try to automatic figure out who the user want the
> > > > TCAM to be configured. It works for 1 use-case but it breaks others.
> > > >
> > > > Before this patch you could do a:
> > > >     tc filter add dev swp0 ingress protocol ipv4 \
> > > >             flower skip_sw src_ip 10.0.0.1 action drop
> > > >     tc filter add dev swp0 ingress \
> > > >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
> > > >
> > > > But the second rule would not apply to the ICMP over IPv4 over Ethernet
> > > > packet, it would however apply to non-IP packets.
> > > >
> > > > With this patch it not possible. Your use-case is more common, but the
> > > > other one is not unrealistic.
> > > >
> > > > My concern with this, is that I do not think it is possible to automatic
> > > > detect how these TCAMs needs to be configured by only looking at the
> > > > rules installed by the user. Trying to do this automatic, also makes the
> > > > TCAM logic even harder to understand for the user.
> > > >
> > > > I would prefer that we by default uses some conservative default
> > > > settings which are easy to understand, and then expose some expert
> > > > settings in the sysfs, which can be used to achieve different
> > > > behavioral.
> > > >
> > > > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> > > > understand default.
> > > >
> > > > But I do seem to recall that there is a way to allow matching on both
> > > > SMAC and SIP (your original motivation). This may be a better default
> > > > (despite that it consumes more TCAM resources). I will follow up and
> > > > check if this is possible.
> > > >
> > > > Vladimir (and anyone else whom interested): would you be interested in
> > > > spending some time discussion the more high-level architectures and
> > > > use-cases on how to best integrate this TCAM architecture into the Linux
> > > > kernel. Not sure on the outlook for the various conferences, but we
> > > > could arrange some online session to discuss this.
> > >
> > > Not sure I completely understand the difficulties you are facing, but it
> > > sounds similar to a problem we had in mlxsw. You might want to look into
> > > "chain templates" [1] in order to restrict the keys that can be used
> > > simultaneously.
> > >
> > > I don't mind participating in an online discussion if you think it can
> > > help.
> > >
> > > [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates
> >
> > I think it is worth giving a bit of context on what motivated me to
> > add this patch. Luckily I believe I can summarize it in a paragraph
> > below.
> >
> > I am trying to understand practical ways in which IEEE 802.1CB can be
> > used - an active redundancy mechanism similar to HSR/PRP which relies
> > on sending sequence-numbered frame duplicates and eliminating those
> > duplicates at the destination (as opposed to passive redundancy
> > mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
> > stop L2 forwarding loops from killing the network). So since 802.1CB
> > needs a network where none of the port states can be put to BLOCKING
> > (as that would break the forwarding of some of the replicated
> > streams), I need a way to limit the impact of L2 loops. Currently I am
> > using, rather successfully, an idea borrowed from HSR called
> > "self-address filtering". It says that received packets can be dropped
> > if their source MAC address matches the device's MAC address. This
> > feature is useful for ensuring that packets never traverse a ring
> > network more than once.
> > To implement this idea, I use an offloaded tc-flower rule matching on
> > src_mac with an action of "drop".
> >
> > To my surprise, such a src_mac rule does not do what's written on the
> > box with the Ocelot switch flow classification engine called VCAP IS2.
> > That is, packets having that src_mac would only get dropped if their
> > protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
> > rule is less than useful for the purpose we want it to.
> > I did raise this concern here, and the suggestion that I got is to
> > implement something like this patch, aka enable a port setting which
> > forces matches on MAC_ETYPE keys only, regardless of higher-layer
> > protocol information:
> > https://lkml.org/lkml/2020/2/24/489
> > So the default (pre-patch) behavior is for IP (and other) matches to
> > be sane, at the expense of MAC matches being insane.
> > Whereas the current behavior is for MAC matches to be sane, at the
> > expense of IP matches becoming impossible as long as MAC rules are
> > also present.
> > In this context, Allan's complaint seems to be that the MAC matches
> > were "good enough" for them, even if not all MAC address matches were
> > caught, at least it did not prevent them from installing IP matching
> > rules on the same port.
> >
> > I may not have completely understood Ido's suggestion to use
> > FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
> > put to practical use to solve the clash here), but I do believe that
> > it is only a way for the driver to eliminate the guesswork out of the
> > user's intention.
>
> I was under the impression that you can't mix different keys (e.g.,
> src_mac + src_ip), but now I understand that you can't mix different
> keys in case of specific key *values*. This will work correctly:
>
> $ tc filter add dev swp0 ingress proto ip \
>         flower src_ip 192.0.2.1 action drop
> $ tc filter add dev swp0 ingress proto 0x88f7 \
>         flower src_mac 00:11:22:33:44:55 action drop
>

Yes.

> This will not work correctly:
>
> $ tc filter add dev swp0 ingress proto ip \
>         flower src_ip 192.0.2.1 action drop
> $ tc filter add dev swp0 ingress proto all \
>         flower src_mac 00:11:22:33:44:55 action drop
>

Yes.

> Correct? If so, I don't think the templates can help you. They are about
> forcing only specific keys, regardless of value.
>

Looking at mlxsw_sp_flower_tmplt_create, I'm not quite sure how it
works (what are the function's effects), but from the verbal
description it doesn't look like they help here.

> > In this case, my personal opinion is that the intention is absolutely
> > clear: a classifier with src_mac should match on all frames having
> > that src_mac (if that is not commonly agreed here, a good rule of
> > thumb is to compare with what a non-offloaded tc filter rule does).
>
> I agree.
>
> > Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
> > to still perform [ without calling ocelot_match_all_as_mac_etype ]
> > should be expressed in terms of a more specific classification key to
> > tc, such as:
>
> Yes.
>
> >
> > tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
> > 96:18:82:00:04:01 action drop
> >
> > In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
> > then these rules can happily live together without ever needing to
> > call ocelot_match_all_as_mac_etype. If we agree on this solution, I
> > can send a patch that refines the ocelot_ace_is_problematic_mac_etype
> > function.
>
> I think it makes sense. You are basically being explicit about the
> hardware limitations and denying configurations that cannot always work.
> Previous approach was to allow configurations that sometimes work.

And current approach is to deny some configurations that do work. I
should fix that.

Thanks,
-Vladimir

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Vladimir Oltean]

On Sun, 19 Apr 2020 at 10:33, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> Hi,
>
> Sorry I did not manage to provide feedback before it was merged (I will
> need to consult some of my colleagues Monday before I can provide the
> foll feedback).
>
> There are many good things in this patch, but it is not only good.
>
> The problem is that these TCAMs/VCAPs are insanely complicated and it is
> really hard to make them fit nicely into the existing tc frame-work
> (being hard does not mean that we should not try).
>
> In this patch, you try to automatic figure out who the user want the
> TCAM to be configured. It works for 1 use-case but it breaks others.
>
> Before this patch you could do a:
>      tc filter add dev swp0 ingress protocol ipv4 \
>              flower skip_sw src_ip 10.0.0.1 action drop
>      tc filter add dev swp0 ingress \
>              flower skip_sw src_mac 96:18:82:00:04:01 action drop
>
> But the second rule would not apply to the ICMP over IPv4 over Ethernet
> packet, it would however apply to non-IP packets.
>
> With this patch it not possible. Your use-case is more common, but the
> other one is not unrealistic.
>
> My concern with this, is that I do not think it is possible to automatic
> detect how these TCAMs needs to be configured by only looking at the
> rules installed by the user. Trying to do this automatic, also makes the
> TCAM logic even harder to understand for the user.
>
> I would prefer that we by default uses some conservative default
> settings which are easy to understand, and then expose some expert
> settings in the sysfs, which can be used to achieve different
> behavioral.
>
> Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> understand default.
>
> But I do seem to recall that there is a way to allow matching on both
> SMAC and SIP (your original motivation). This may be a better default
> (despite that it consumes more TCAM resources). I will follow up and
> check if this is possible.
>
> Vladimir (and anyone else whom interested): would you be interested in
> spending some time discussion the more high-level architectures and
> use-cases on how to best integrate this TCAM architecture into the Linux
> kernel. Not sure on the outlook for the various conferences, but we
> could arrange some online session to discuss this.
>
> /Allan
>

And yes, we would be very interested in attending a call for syncing
up on integrating the TCAM hardware with the flow offload
infrastructure from Linux. Actually at the moment we are trying to add
support for offloaded VLAN retagging with the VCAP IS1 and ES0 blocks.

Thanks,
-Vladimir

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Allan W. Nielsen]

On 19.04.2020 11:30, Ido Schimmel wrote:
>Not sure I completely understand the difficulties you are facing, but it
>sounds similar to a problem we had in mlxsw. You might want to look into
>"chain templates" [1] in order to restrict the keys that can be used
>simultaneously.
Not sure I understood the details, but but sure it is the same issue we
are trying to solve.

>I don't mind participating in an online discussion if you think it can
>help.
I'm sure it would be helpfull to have someone with insight in the MLX
driver. I have been looking a lot at it, and there are large part of it
which I still have not understood.

If you get too borrowed you can always leave ;-)

/Allan

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Allan W. Nielsen]

On 19.04.2020 15:47, Vladimir Oltean wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>Hi Ido, Allan,
>
>On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@idosch.org> wrote:
>>
>> On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
>> > Hi,
>> >
>> > Sorry I did not manage to provide feedback before it was merged (I will
>> > need to consult some of my colleagues Monday before I can provide the
>> > foll feedback).
>> >
>> > There are many good things in this patch, but it is not only good.
>> >
>> > The problem is that these TCAMs/VCAPs are insanely complicated and it is
>> > really hard to make them fit nicely into the existing tc frame-work
>> > (being hard does not mean that we should not try).
>> >
>> > In this patch, you try to automatic figure out who the user want the
>> > TCAM to be configured. It works for 1 use-case but it breaks others.
>> >
>> > Before this patch you could do a:
>> >     tc filter add dev swp0 ingress protocol ipv4 \
>> >             flower skip_sw src_ip 10.0.0.1 action drop
>> >     tc filter add dev swp0 ingress \
>> >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
>> >
>> > But the second rule would not apply to the ICMP over IPv4 over Ethernet
>> > packet, it would however apply to non-IP packets.
>> >
>> > With this patch it not possible. Your use-case is more common, but the
>> > other one is not unrealistic.
>> >
>> > My concern with this, is that I do not think it is possible to automatic
>> > detect how these TCAMs needs to be configured by only looking at the
>> > rules installed by the user. Trying to do this automatic, also makes the
>> > TCAM logic even harder to understand for the user.
>> >
>> > I would prefer that we by default uses some conservative default
>> > settings which are easy to understand, and then expose some expert
>> > settings in the sysfs, which can be used to achieve different
>> > behavioral.
>> >
>> > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
>> > understand default.
>> >
>> > But I do seem to recall that there is a way to allow matching on both
>> > SMAC and SIP (your original motivation). This may be a better default
>> > (despite that it consumes more TCAM resources). I will follow up and
>> > check if this is possible.
>> >
>> > Vladimir (and anyone else whom interested): would you be interested in
>> > spending some time discussion the more high-level architectures and
>> > use-cases on how to best integrate this TCAM architecture into the Linux
>> > kernel. Not sure on the outlook for the various conferences, but we
>> > could arrange some online session to discuss this.
>>
>> Not sure I completely understand the difficulties you are facing, but it
>> sounds similar to a problem we had in mlxsw. You might want to look into
>> "chain templates" [1] in order to restrict the keys that can be used
>> simultaneously.
>>
>> I don't mind participating in an online discussion if you think it can
>> help.
>>
>> [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates
>
>I think it is worth giving a bit of context on what motivated me to
>add this patch. Luckily I believe I can summarize it in a paragraph
>below.
>
>I am trying to understand practical ways in which IEEE 802.1CB can be
>used - an active redundancy mechanism similar to HSR/PRP which relies
>on sending sequence-numbered frame duplicates and eliminating those
>duplicates at the destination (as opposed to passive redundancy
>mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
>stop L2 forwarding loops from killing the network). So since 802.1CB
>needs a network where none of the port states can be put to BLOCKING
>(as that would break the forwarding of some of the replicated
>streams), I need a way to limit the impact of L2 loops. Currently I am
>using, rather successfully, an idea borrowed from HSR called
>"self-address filtering". It says that received packets can be dropped
>if their source MAC address matches the device's MAC address. This
>feature is useful for ensuring that packets never traverse a ring
>network more than once.
>To implement this idea, I use an offloaded tc-flower rule matching on
>src_mac with an action of "drop".
>
>To my surprise, such a src_mac rule does not do what's written on the
>box with the Ocelot switch flow classification engine called VCAP IS2.
>That is, packets having that src_mac would only get dropped if their
>protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
>rule is less than useful for the purpose we want it to.
>I did raise this concern here, and the suggestion that I got is to
>implement something like this patch, aka enable a port setting which
>forces matches on MAC_ETYPE keys only, regardless of higher-layer
>protocol information:
>https://lkml.org/lkml/2020/2/24/489
>So the default (pre-patch) behavior is for IP (and other) matches to
>be sane, at the expense of MAC matches being insane.
>Whereas the current behavior is for MAC matches to be sane, at the
>expense of IP matches becoming impossible as long as MAC rules are
>also present.
>In this context, Allan's complaint seems to be that the MAC matches
>were "good enough" for them, even if not all MAC address matches were
>caught, at least it did not prevent them from installing IP matching
>rules on the same port.
The truth is rather that what we have of now is still not capable
enough to solve the problems we need. This is why I'm keen on
discussion/brainstorm how more complicated scenarios can be supported.

>I may not have completely understood Ido's suggestion to use
>FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
>put to practical use to solve the clash here), but I do believe that
>it is only a way for the driver to eliminate the guesswork out of the
>user's intention.
>In this case, my personal opinion is that the intention is absolutely
>clear: a classifier with src_mac should match on all frames having
>that src_mac (if that is not commonly agreed here, a good rule of
>thumb is to compare with what a non-offloaded tc filter rule does).
I think this is a good default behavioral.

But there are cases where people might want something different.

Also, here we are talking about a relatively small fraction of the TCAM
facilities in Ocelot/Felix. What I would like to is to consider the
entire system. Considering Felix it would be great if all these extended
(features implemented in user-space) could be fitted into the kernel.

>Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
>to still perform [ without calling ocelot_match_all_as_mac_etype ]
>should be expressed in terms of a more specific classification key to
>tc, such as:
>
>tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
>96:18:82:00:04:01 action drop
>
>In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
>then these rules can happily live together without ever needing to
>call ocelot_match_all_as_mac_etype. If we agree on this solution, I
>can send a patch that refines the ocelot_ace_is_problematic_mac_etype
>function.
>
>Thanks,
>-Vladimir

/Allan

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Allan W. Nielsen]

On 19.04.2020 17:20, Vladimir Oltean wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Sun, 19 Apr 2020 at 10:33, Allan W. Nielsen
><allan.nielsen@microchip.com> wrote:
>>
>> Hi,
>>
>> Sorry I did not manage to provide feedback before it was merged (I will
>> need to consult some of my colleagues Monday before I can provide the
>> foll feedback).
>>
>> There are many good things in this patch, but it is not only good.
>>
>> The problem is that these TCAMs/VCAPs are insanely complicated and it is
>> really hard to make them fit nicely into the existing tc frame-work
>> (being hard does not mean that we should not try).
>>
>> In this patch, you try to automatic figure out who the user want the
>> TCAM to be configured. It works for 1 use-case but it breaks others.
>>
>> Before this patch you could do a:
>>      tc filter add dev swp0 ingress protocol ipv4 \
>>              flower skip_sw src_ip 10.0.0.1 action drop
>>      tc filter add dev swp0 ingress \
>>              flower skip_sw src_mac 96:18:82:00:04:01 action drop
>>
>> But the second rule would not apply to the ICMP over IPv4 over Ethernet
>> packet, it would however apply to non-IP packets.
>>
>> With this patch it not possible. Your use-case is more common, but the
>> other one is not unrealistic.
>>
>> My concern with this, is that I do not think it is possible to automatic
>> detect how these TCAMs needs to be configured by only looking at the
>> rules installed by the user. Trying to do this automatic, also makes the
>> TCAM logic even harder to understand for the user.
>>
>> I would prefer that we by default uses some conservative default
>> settings which are easy to understand, and then expose some expert
>> settings in the sysfs, which can be used to achieve different
>> behavioral.
>>
>> Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
>> understand default.
>>
>> But I do seem to recall that there is a way to allow matching on both
>> SMAC and SIP (your original motivation). This may be a better default
>> (despite that it consumes more TCAM resources). I will follow up and
>> check if this is possible.
>>
>> Vladimir (and anyone else whom interested): would you be interested in
>> spending some time discussion the more high-level architectures and
>> use-cases on how to best integrate this TCAM architecture into the Linux
>> kernel. Not sure on the outlook for the various conferences, but we
>> could arrange some online session to discuss this.
>>
>> /Allan
>>
>
>And yes, we would be very interested in attending a call for syncing
>up on integrating the TCAM hardware with the flow offload
>infrastructure from Linux. Actually at the moment we are trying to add
>support for offloaded VLAN retagging with the VCAP IS1 and ES0 blocks.

Sounds good - lets spend some time to talk discuss this and see what
comes out of it.

Ido, if you want to join us, pleaes comment with your preferences. If
others want to join please let me know.

I can setup a meeting in WebEx or Teams. I'm happy to join on other
platformws if you prefer. They both works fine from Linux in Chrome and
FireFox (sometimes tricky to get the sound working in FF).

Proposed agenda:

- Cover the TCAM architecture in Ocelot/Felix (just to make sure we are
   all on the same page).
- Present some use-cases MCHP would like to address in future.
- Open discussion.

I think we will need something between 30-120 minutes depending on how
the discussion goes.

We are in CEST time - and I'm okay to attend from 7-22.

What about you.

/Allan

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Vladimir Oltean]

Hi Allan,

On Sun, 19 Apr 2020 at 21:25, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> On 19.04.2020 17:20, Vladimir Oltean wrote:
> >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >On Sun, 19 Apr 2020 at 10:33, Allan W. Nielsen
> ><allan.nielsen@microchip.com> wrote:
> >>
> >> Hi,
> >>
> >> Sorry I did not manage to provide feedback before it was merged (I will
> >> need to consult some of my colleagues Monday before I can provide the
> >> foll feedback).
> >>
> >> There are many good things in this patch, but it is not only good.
> >>
> >> The problem is that these TCAMs/VCAPs are insanely complicated and it is
> >> really hard to make them fit nicely into the existing tc frame-work
> >> (being hard does not mean that we should not try).
> >>
> >> In this patch, you try to automatic figure out who the user want the
> >> TCAM to be configured. It works for 1 use-case but it breaks others.
> >>
> >> Before this patch you could do a:
> >>      tc filter add dev swp0 ingress protocol ipv4 \
> >>              flower skip_sw src_ip 10.0.0.1 action drop
> >>      tc filter add dev swp0 ingress \
> >>              flower skip_sw src_mac 96:18:82:00:04:01 action drop
> >>
> >> But the second rule would not apply to the ICMP over IPv4 over Ethernet
> >> packet, it would however apply to non-IP packets.
> >>
> >> With this patch it not possible. Your use-case is more common, but the
> >> other one is not unrealistic.
> >>
> >> My concern with this, is that I do not think it is possible to automatic
> >> detect how these TCAMs needs to be configured by only looking at the
> >> rules installed by the user. Trying to do this automatic, also makes the
> >> TCAM logic even harder to understand for the user.
> >>
> >> I would prefer that we by default uses some conservative default
> >> settings which are easy to understand, and then expose some expert
> >> settings in the sysfs, which can be used to achieve different
> >> behavioral.
> >>
> >> Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
> >> understand default.
> >>
> >> But I do seem to recall that there is a way to allow matching on both
> >> SMAC and SIP (your original motivation). This may be a better default
> >> (despite that it consumes more TCAM resources). I will follow up and
> >> check if this is possible.
> >>
> >> Vladimir (and anyone else whom interested): would you be interested in
> >> spending some time discussion the more high-level architectures and
> >> use-cases on how to best integrate this TCAM architecture into the Linux
> >> kernel. Not sure on the outlook for the various conferences, but we
> >> could arrange some online session to discuss this.
> >>
> >> /Allan
> >>
> >
> >And yes, we would be very interested in attending a call for syncing
> >up on integrating the TCAM hardware with the flow offload
> >infrastructure from Linux. Actually at the moment we are trying to add
> >support for offloaded VLAN retagging with the VCAP IS1 and ES0 blocks.
>
> Sounds good - lets spend some time to talk discuss this and see what
> comes out of it.
>
> Ido, if you want to join us, pleaes comment with your preferences. If
> others want to join please let me know.
>
> I can setup a meeting in WebEx or Teams. I'm happy to join on other
> platformws if you prefer. They both works fine from Linux in Chrome and
> FireFox (sometimes tricky to get the sound working in FF).
>
> Proposed agenda:
>
> - Cover the TCAM architecture in Ocelot/Felix (just to make sure we are
>    all on the same page).
> - Present some use-cases MCHP would like to address in future.
> - Open discussion.
>
> I think we will need something between 30-120 minutes depending on how
> the discussion goes.
>
> We are in CEST time - and I'm okay to attend from 7-22.
>
> What about you.
>
> /Allan

From my side I am available for the entire time interval you
mentioned, since the time zone in Bucharest (GMT+3) is rather close to
Copenhagen. Our colleagues from NXP Beijing might also be interested,
which is probably going to restrict the time interval to the first
half of the day. And you can also schedule for Tuesday if tomorrow is
on too short notice.
Both WebEx and Teams should work, with a slight preference to Teams
since NXP people are already using it.

Thanks,
-Vladimir

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Ido Schimmel]

On Sun, Apr 19, 2020 at 08:25:34PM +0200, Allan W. Nielsen wrote:
> Sounds good - lets spend some time to talk discuss this and see what
> comes out of it.
> 
> Ido, if you want to join us, pleaes comment with your preferences. If
> others want to join please let me know.
> 
> I can setup a meeting in WebEx or Teams. I'm happy to join on other
> platformws if you prefer. They both works fine from Linux in Chrome and
> FireFox (sometimes tricky to get the sound working in FF).
> 
> Proposed agenda:
> 
> - Cover the TCAM architecture in Ocelot/Felix (just to make sure we are
>   all on the same page).
> - Present some use-cases MCHP would like to address in future.
> - Open discussion.
> 
> I think we will need something between 30-120 minutes depending on how
> the discussion goes.
> 
> We are in CEST time - and I'm okay to attend from 7-22.

I'm in GMT+3 like Vladimir. Don't mind if it's WebEx or Teams. Jiri
might join as well, so please send the invitation to him as well.

Thanks!

Re: [PATCH net-next] net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules

[Jiri Pirko]

Sun, Apr 19, 2020 at 03:51:43PM CEST, idosch@idosch.org wrote:
>On Sun, Apr 19, 2020 at 03:47:01PM +0300, Vladimir Oltean wrote:
>> Hi Ido, Allan,
>> 
>> On Sun, 19 Apr 2020 at 11:30, Ido Schimmel <idosch@idosch.org> wrote:
>> >
>> > On Sun, Apr 19, 2020 at 09:33:07AM +0200, Allan W. Nielsen wrote:
>> > > Hi,
>> > >
>> > > Sorry I did not manage to provide feedback before it was merged (I will
>> > > need to consult some of my colleagues Monday before I can provide the
>> > > foll feedback).
>> > >
>> > > There are many good things in this patch, but it is not only good.
>> > >
>> > > The problem is that these TCAMs/VCAPs are insanely complicated and it is
>> > > really hard to make them fit nicely into the existing tc frame-work
>> > > (being hard does not mean that we should not try).
>> > >
>> > > In this patch, you try to automatic figure out who the user want the
>> > > TCAM to be configured. It works for 1 use-case but it breaks others.
>> > >
>> > > Before this patch you could do a:
>> > >     tc filter add dev swp0 ingress protocol ipv4 \
>> > >             flower skip_sw src_ip 10.0.0.1 action drop
>> > >     tc filter add dev swp0 ingress \
>> > >             flower skip_sw src_mac 96:18:82:00:04:01 action drop
>> > >
>> > > But the second rule would not apply to the ICMP over IPv4 over Ethernet
>> > > packet, it would however apply to non-IP packets.
>> > >
>> > > With this patch it not possible. Your use-case is more common, but the
>> > > other one is not unrealistic.
>> > >
>> > > My concern with this, is that I do not think it is possible to automatic
>> > > detect how these TCAMs needs to be configured by only looking at the
>> > > rules installed by the user. Trying to do this automatic, also makes the
>> > > TCAM logic even harder to understand for the user.
>> > >
>> > > I would prefer that we by default uses some conservative default
>> > > settings which are easy to understand, and then expose some expert
>> > > settings in the sysfs, which can be used to achieve different
>> > > behavioral.
>> > >
>> > > Maybe forcing MAC_ETYPE matches is the most conservative and easiest to
>> > > understand default.
>> > >
>> > > But I do seem to recall that there is a way to allow matching on both
>> > > SMAC and SIP (your original motivation). This may be a better default
>> > > (despite that it consumes more TCAM resources). I will follow up and
>> > > check if this is possible.
>> > >
>> > > Vladimir (and anyone else whom interested): would you be interested in
>> > > spending some time discussion the more high-level architectures and
>> > > use-cases on how to best integrate this TCAM architecture into the Linux
>> > > kernel. Not sure on the outlook for the various conferences, but we
>> > > could arrange some online session to discuss this.
>> >
>> > Not sure I completely understand the difficulties you are facing, but it
>> > sounds similar to a problem we had in mlxsw. You might want to look into
>> > "chain templates" [1] in order to restrict the keys that can be used
>> > simultaneously.
>> >
>> > I don't mind participating in an online discussion if you think it can
>> > help.
>> >
>> > [1] https://github.com/Mellanox/mlxsw/wiki/ACLs#chain-templates
>> 
>> I think it is worth giving a bit of context on what motivated me to
>> add this patch. Luckily I believe I can summarize it in a paragraph
>> below.
>> 
>> I am trying to understand practical ways in which IEEE 802.1CB can be
>> used - an active redundancy mechanism similar to HSR/PRP which relies
>> on sending sequence-numbered frame duplicates and eliminating those
>> duplicates at the destination (as opposed to passive redundancy
>> mechanisms such as RSTP, MRP etc which rely on BLOCKING port states to
>> stop L2 forwarding loops from killing the network). So since 802.1CB
>> needs a network where none of the port states can be put to BLOCKING
>> (as that would break the forwarding of some of the replicated
>> streams), I need a way to limit the impact of L2 loops. Currently I am
>> using, rather successfully, an idea borrowed from HSR called
>> "self-address filtering". It says that received packets can be dropped
>> if their source MAC address matches the device's MAC address. This
>> feature is useful for ensuring that packets never traverse a ring
>> network more than once.
>> To implement this idea, I use an offloaded tc-flower rule matching on
>> src_mac with an action of "drop".
>> 
>> To my surprise, such a src_mac rule does not do what's written on the
>> box with the Ocelot switch flow classification engine called VCAP IS2.
>> That is, packets having that src_mac would only get dropped if their
>> protocol is not ARP, SNAP, IPv4, IPv6 and maybe others. Clearly such a
>> rule is less than useful for the purpose we want it to.
>> I did raise this concern here, and the suggestion that I got is to
>> implement something like this patch, aka enable a port setting which
>> forces matches on MAC_ETYPE keys only, regardless of higher-layer
>> protocol information:
>> https://lkml.org/lkml/2020/2/24/489
>> So the default (pre-patch) behavior is for IP (and other) matches to
>> be sane, at the expense of MAC matches being insane.
>> Whereas the current behavior is for MAC matches to be sane, at the
>> expense of IP matches becoming impossible as long as MAC rules are
>> also present.
>> In this context, Allan's complaint seems to be that the MAC matches
>> were "good enough" for them, even if not all MAC address matches were
>> caught, at least it did not prevent them from installing IP matching
>> rules on the same port.
>> 
>> I may not have completely understood Ido's suggestion to use
>> FLOW_CLS_TMPLT_CREATE (I might lack the imagination of how it can be
>> put to practical use to solve the clash here), but I do believe that
>> it is only a way for the driver to eliminate the guesswork out of the
>> user's intention.
>
>I was under the impression that you can't mix different keys (e.g.,
>src_mac + src_ip), but now I understand that you can't mix different
>keys in case of specific key *values*. This will work correctly:
>
>$ tc filter add dev swp0 ingress proto ip \
>	flower src_ip 192.0.2.1 action drop
>$ tc filter add dev swp0 ingress proto 0x88f7 \
>	flower src_mac 00:11:22:33:44:55 action drop
>
>This will not work correctly:
>
>$ tc filter add dev swp0 ingress proto ip \
>	flower src_ip 192.0.2.1 action drop
>$ tc filter add dev swp0 ingress proto all \
>	flower src_mac 00:11:22:33:44:55 action drop

If this screnario is not supported by the driver, it is always free to
return -EOPNOTSUPP.


>
>Correct? If so, I don't think the templates can help you. They are about
>forcing only specific keys, regardless of value.
>
>> In this case, my personal opinion is that the intention is absolutely
>> clear: a classifier with src_mac should match on all frames having
>> that src_mac (if that is not commonly agreed here, a good rule of
>> thumb is to compare with what a non-offloaded tc filter rule does).
>
>I agree.
>
>> Whereas the "non-problematic" MAC matches that the VCAP IS2 _is_ able
>> to still perform [ without calling ocelot_match_all_as_mac_etype ]
>> should be expressed in terms of a more specific classification key to
>> tc, such as:
>
>Yes.
>
>> 
>> tc filter add dev swp0 ingress *protocol 0x88f7* flower src_mac
>> 96:18:82:00:04:01 action drop
>> 
>> In the above case, because "protocol" is not ipv4, ipv6, arp, snap,
>> then these rules can happily live together without ever needing to
>> call ocelot_match_all_as_mac_etype. If we agree on this solution, I
>> can send a patch that refines the ocelot_ace_is_problematic_mac_etype
>> function.
>
>I think it makes sense. You are basically being explicit about the
>hardware limitations and denying configurations that cannot always work.
>Previous approach was to allow configurations that sometimes work.