[PATCH net-next 0/3] Ocelot MAC_ETYPE tc-flower key improvements

[PATCH net-next 0/3] Ocelot MAC_ETYPE tc-flower key improvements

[Vladimir Oltean]

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

As discussed in the comments surrounding this patch:
https://patchwork.ozlabs.org/project/netdev/patch/20200417190308.32598-1-olteanv@gmail.com/

the restrictions imposed on non-MAC_ETYPE rules were harsher than they
needed to be. IP, IPv6, ARP rules can still be added concurrently with
src_mac and dst_mac rules, as long as those MAC address rules do not ask
for an offending EtherType.

For that to actually be supported, we need to parse the EtherType from
the flower classification rule first.

Vladimir Oltean (3):
  net: mscc: ocelot: support matching on EtherType
  net: mscc: ocelot: refine the ocelot_ace_is_problematic_mac_etype
    function
  net: mscc: ocelot: lift protocol restriction for flow_match_eth_addrs
    keys

 drivers/net/ethernet/mscc/ocelot_ace.c    | 18 +++++++++++----
 drivers/net/ethernet/mscc/ocelot_flower.c | 27 ++++++++++++++++-------
 2 files changed, 33 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[Vladimir Oltean]

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

Currently, the filter's protocol is ignored except for a few special
cases (IPv4 and IPv6).

The EtherType can be matched inside VCAP IS2 by using a MAC_ETYPE key.
So there are 2 cases in which EtherType matches are supported:

  - As part of a larger MAC_ETYPE rule, such as:

    tc filter add dev swp0 ingress protocol ip \
            flower skip_sw src_mac 42:be:24:9b:76:20 action drop

  - Standalone (matching on protocol only):

    tc filter add dev swp0 ingress protocol arp \
            flower skip_sw action drop

As before, if the protocol is not specified, is it implicitly "all" and
the EtherType mask in the MAC_ETYPE half key is set to zero.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 954cb67eeaa2..67f0f5455ff0 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -51,6 +51,8 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 {
 	struct flow_rule *rule = flow_cls_offload_flow_rule(f);
 	struct flow_dissector *dissector = rule->match.dissector;
+	u16 proto = ntohs(f->common.protocol);
+	bool match_protocol = true;
 
 	if (dissector->used_keys &
 	    ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
@@ -71,7 +73,6 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
 		struct flow_match_eth_addrs match;
-		u16 proto = ntohs(f->common.protocol);
 
 		/* The hw support mac matches only for MAC_ETYPE key,
 		 * therefore if other matches(port, tcp flags, etc) are added
@@ -114,6 +115,7 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 				match.key->ip_proto;
 			ace->frame.ipv4.proto.mask[0] =
 				match.mask->ip_proto;
+			match_protocol = false;
 		}
 		if (ntohs(match.key->n_proto) == ETH_P_IPV6) {
 			ace->type = OCELOT_ACE_TYPE_IPV6;
@@ -121,11 +123,12 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 				match.key->ip_proto;
 			ace->frame.ipv6.proto.mask[0] =
 				match.mask->ip_proto;
+			match_protocol = false;
 		}
 	}
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS) &&
-	    ntohs(f->common.protocol) == ETH_P_IP) {
+	    proto == ETH_P_IP) {
 		struct flow_match_ipv4_addrs match;
 		u8 *tmp;
 
@@ -141,10 +144,11 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 
 		tmp = &ace->frame.ipv4.dip.mask.addr[0];
 		memcpy(tmp, &match.mask->dst, 4);
+		match_protocol = false;
 	}
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS) &&
-	    ntohs(f->common.protocol) == ETH_P_IPV6) {
+	    proto == ETH_P_IPV6) {
 		return -EOPNOTSUPP;
 	}
 
@@ -156,6 +160,7 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 		ace->frame.ipv4.sport.mask = ntohs(match.mask->src);
 		ace->frame.ipv4.dport.value = ntohs(match.key->dst);
 		ace->frame.ipv4.dport.mask = ntohs(match.mask->dst);
+		match_protocol = false;
 	}
 
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
@@ -167,9 +172,20 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 		ace->vlan.vid.mask = match.mask->vlan_id;
 		ace->vlan.pcp.value[0] = match.key->vlan_priority;
 		ace->vlan.pcp.mask[0] = match.mask->vlan_priority;
+		match_protocol = false;
 	}
 
 finished_key_parsing:
+	if (match_protocol && proto != ETH_P_ALL) {
+		/* TODO: support SNAP, LLC etc */
+		if (proto < ETH_P_802_3_MIN)
+			return -EOPNOTSUPP;
+		ace->type = OCELOT_ACE_TYPE_ETYPE;
+		*(u16 *)ace->frame.etype.etype.value = htons(proto);
+		*(u16 *)ace->frame.etype.etype.mask = 0xffff;
+	}
+	/* else, a rule of type OCELOT_ACE_TYPE_ANY is implicitly added */
+
 	ace->prio = f->common.prio;
 	ace->id = f->cookie;
 	return ocelot_flower_parse_action(f, ace);
-- 
2.17.1

Re: [PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[kbuild test robot]

Hi Vladimir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v5.7-rc2 next-20200421]
[cannot apply to sparc-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Ocelot-MAC_ETYPE-tc-flower-key-improvements/20200422-113906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b6246f4d8d0778fd045b84dbd7fc5aadd8f3136e
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [usertype] @@    got resunsigned short [usertype] @@
>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    expected unsigned short [usertype]
>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    got restricted __be16 [usertype]

vim +184 drivers/net/ethernet/mscc/ocelot_flower.c

    48	
    49	static int ocelot_flower_parse(struct flow_cls_offload *f,
    50				       struct ocelot_ace_rule *ace)
    51	{
    52		struct flow_rule *rule = flow_cls_offload_flow_rule(f);
    53		struct flow_dissector *dissector = rule->match.dissector;
    54		u16 proto = ntohs(f->common.protocol);
    55		bool match_protocol = true;
    56	
    57		if (dissector->used_keys &
    58		    ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
    59		      BIT(FLOW_DISSECTOR_KEY_BASIC) |
    60		      BIT(FLOW_DISSECTOR_KEY_PORTS) |
    61		      BIT(FLOW_DISSECTOR_KEY_VLAN) |
    62		      BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
    63		      BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
    64		      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS))) {
    65			return -EOPNOTSUPP;
    66		}
    67	
    68		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
    69			struct flow_match_control match;
    70	
    71			flow_rule_match_control(rule, &match);
    72		}
    73	
    74		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
    75			struct flow_match_eth_addrs match;
    76	
    77			/* The hw support mac matches only for MAC_ETYPE key,
    78			 * therefore if other matches(port, tcp flags, etc) are added
    79			 * then just bail out
    80			 */
    81			if ((dissector->used_keys &
    82			    (BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
    83			     BIT(FLOW_DISSECTOR_KEY_BASIC) |
    84			     BIT(FLOW_DISSECTOR_KEY_CONTROL))) !=
    85			    (BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
    86			     BIT(FLOW_DISSECTOR_KEY_BASIC) |
    87			     BIT(FLOW_DISSECTOR_KEY_CONTROL)))
    88				return -EOPNOTSUPP;
    89	
    90			if (proto == ETH_P_IP ||
    91			    proto == ETH_P_IPV6 ||
    92			    proto == ETH_P_ARP)
    93				return -EOPNOTSUPP;
    94	
    95			flow_rule_match_eth_addrs(rule, &match);
    96			ace->type = OCELOT_ACE_TYPE_ETYPE;
    97			ether_addr_copy(ace->frame.etype.dmac.value,
    98					match.key->dst);
    99			ether_addr_copy(ace->frame.etype.smac.value,
   100					match.key->src);
   101			ether_addr_copy(ace->frame.etype.dmac.mask,
   102					match.mask->dst);
   103			ether_addr_copy(ace->frame.etype.smac.mask,
   104					match.mask->src);
   105			goto finished_key_parsing;
   106		}
   107	
   108		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
   109			struct flow_match_basic match;
   110	
   111			flow_rule_match_basic(rule, &match);
   112			if (ntohs(match.key->n_proto) == ETH_P_IP) {
   113				ace->type = OCELOT_ACE_TYPE_IPV4;
   114				ace->frame.ipv4.proto.value[0] =
   115					match.key->ip_proto;
   116				ace->frame.ipv4.proto.mask[0] =
   117					match.mask->ip_proto;
   118				match_protocol = false;
   119			}
   120			if (ntohs(match.key->n_proto) == ETH_P_IPV6) {
   121				ace->type = OCELOT_ACE_TYPE_IPV6;
   122				ace->frame.ipv6.proto.value[0] =
   123					match.key->ip_proto;
   124				ace->frame.ipv6.proto.mask[0] =
   125					match.mask->ip_proto;
   126				match_protocol = false;
   127			}
   128		}
   129	
   130		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS) &&
   131		    proto == ETH_P_IP) {
   132			struct flow_match_ipv4_addrs match;
   133			u8 *tmp;
   134	
   135			flow_rule_match_ipv4_addrs(rule, &match);
   136			tmp = &ace->frame.ipv4.sip.value.addr[0];
   137			memcpy(tmp, &match.key->src, 4);
   138	
   139			tmp = &ace->frame.ipv4.sip.mask.addr[0];
   140			memcpy(tmp, &match.mask->src, 4);
   141	
   142			tmp = &ace->frame.ipv4.dip.value.addr[0];
   143			memcpy(tmp, &match.key->dst, 4);
   144	
   145			tmp = &ace->frame.ipv4.dip.mask.addr[0];
   146			memcpy(tmp, &match.mask->dst, 4);
   147			match_protocol = false;
   148		}
   149	
   150		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS) &&
   151		    proto == ETH_P_IPV6) {
   152			return -EOPNOTSUPP;
   153		}
   154	
   155		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PORTS)) {
   156			struct flow_match_ports match;
   157	
   158			flow_rule_match_ports(rule, &match);
   159			ace->frame.ipv4.sport.value = ntohs(match.key->src);
   160			ace->frame.ipv4.sport.mask = ntohs(match.mask->src);
   161			ace->frame.ipv4.dport.value = ntohs(match.key->dst);
   162			ace->frame.ipv4.dport.mask = ntohs(match.mask->dst);
   163			match_protocol = false;
   164		}
   165	
   166		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
   167			struct flow_match_vlan match;
   168	
   169			flow_rule_match_vlan(rule, &match);
   170			ace->type = OCELOT_ACE_TYPE_ANY;
   171			ace->vlan.vid.value = match.key->vlan_id;
   172			ace->vlan.vid.mask = match.mask->vlan_id;
   173			ace->vlan.pcp.value[0] = match.key->vlan_priority;
   174			ace->vlan.pcp.mask[0] = match.mask->vlan_priority;
   175			match_protocol = false;
   176		}
   177	
   178	finished_key_parsing:
   179		if (match_protocol && proto != ETH_P_ALL) {
   180			/* TODO: support SNAP, LLC etc */
   181			if (proto < ETH_P_802_3_MIN)
   182				return -EOPNOTSUPP;
   183			ace->type = OCELOT_ACE_TYPE_ETYPE;
 > 184			*(u16 *)ace->frame.etype.etype.value = htons(proto);
   185			*(u16 *)ace->frame.etype.etype.mask = 0xffff;
   186		}
   187		/* else, a rule of type OCELOT_ACE_TYPE_ANY is implicitly added */
   188	
   189		ace->prio = f->common.prio;
   190		ace->id = f->cookie;
   191		return ocelot_flower_parse_action(f, ace);
   192	}
   193	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7148 bytes --]

Hi Vladimir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master v5.7-rc2 next-20200421]
[cannot apply to sparc-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Ocelot-MAC_ETYPE-tc-flower-key-improvements/20200422-113906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b6246f4d8d0778fd045b84dbd7fc5aadd8f3136e
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [usertype] @@    got resunsigned short [usertype] @@
>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    expected unsigned short [usertype]
>> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    got restricted __be16 [usertype]

vim +184 drivers/net/ethernet/mscc/ocelot_flower.c

    48	
    49	static int ocelot_flower_parse(struct flow_cls_offload *f,
    50				       struct ocelot_ace_rule *ace)
    51	{
    52		struct flow_rule *rule = flow_cls_offload_flow_rule(f);
    53		struct flow_dissector *dissector = rule->match.dissector;
    54		u16 proto = ntohs(f->common.protocol);
    55		bool match_protocol = true;
    56	
    57		if (dissector->used_keys &
    58		    ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
    59		      BIT(FLOW_DISSECTOR_KEY_BASIC) |
    60		      BIT(FLOW_DISSECTOR_KEY_PORTS) |
    61		      BIT(FLOW_DISSECTOR_KEY_VLAN) |
    62		      BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
    63		      BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
    64		      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS))) {
    65			return -EOPNOTSUPP;
    66		}
    67	
    68		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
    69			struct flow_match_control match;
    70	
    71			flow_rule_match_control(rule, &match);
    72		}
    73	
    74		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
    75			struct flow_match_eth_addrs match;
    76	
    77			/* The hw support mac matches only for MAC_ETYPE key,
    78			 * therefore if other matches(port, tcp flags, etc) are added
    79			 * then just bail out
    80			 */
    81			if ((dissector->used_keys &
    82			    (BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
    83			     BIT(FLOW_DISSECTOR_KEY_BASIC) |
    84			     BIT(FLOW_DISSECTOR_KEY_CONTROL))) !=
    85			    (BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
    86			     BIT(FLOW_DISSECTOR_KEY_BASIC) |
    87			     BIT(FLOW_DISSECTOR_KEY_CONTROL)))
    88				return -EOPNOTSUPP;
    89	
    90			if (proto == ETH_P_IP ||
    91			    proto == ETH_P_IPV6 ||
    92			    proto == ETH_P_ARP)
    93				return -EOPNOTSUPP;
    94	
    95			flow_rule_match_eth_addrs(rule, &match);
    96			ace->type = OCELOT_ACE_TYPE_ETYPE;
    97			ether_addr_copy(ace->frame.etype.dmac.value,
    98					match.key->dst);
    99			ether_addr_copy(ace->frame.etype.smac.value,
   100					match.key->src);
   101			ether_addr_copy(ace->frame.etype.dmac.mask,
   102					match.mask->dst);
   103			ether_addr_copy(ace->frame.etype.smac.mask,
   104					match.mask->src);
   105			goto finished_key_parsing;
   106		}
   107	
   108		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
   109			struct flow_match_basic match;
   110	
   111			flow_rule_match_basic(rule, &match);
   112			if (ntohs(match.key->n_proto) == ETH_P_IP) {
   113				ace->type = OCELOT_ACE_TYPE_IPV4;
   114				ace->frame.ipv4.proto.value[0] =
   115					match.key->ip_proto;
   116				ace->frame.ipv4.proto.mask[0] =
   117					match.mask->ip_proto;
   118				match_protocol = false;
   119			}
   120			if (ntohs(match.key->n_proto) == ETH_P_IPV6) {
   121				ace->type = OCELOT_ACE_TYPE_IPV6;
   122				ace->frame.ipv6.proto.value[0] =
   123					match.key->ip_proto;
   124				ace->frame.ipv6.proto.mask[0] =
   125					match.mask->ip_proto;
   126				match_protocol = false;
   127			}
   128		}
   129	
   130		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS) &&
   131		    proto == ETH_P_IP) {
   132			struct flow_match_ipv4_addrs match;
   133			u8 *tmp;
   134	
   135			flow_rule_match_ipv4_addrs(rule, &match);
   136			tmp = &ace->frame.ipv4.sip.value.addr[0];
   137			memcpy(tmp, &match.key->src, 4);
   138	
   139			tmp = &ace->frame.ipv4.sip.mask.addr[0];
   140			memcpy(tmp, &match.mask->src, 4);
   141	
   142			tmp = &ace->frame.ipv4.dip.value.addr[0];
   143			memcpy(tmp, &match.key->dst, 4);
   144	
   145			tmp = &ace->frame.ipv4.dip.mask.addr[0];
   146			memcpy(tmp, &match.mask->dst, 4);
   147			match_protocol = false;
   148		}
   149	
   150		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV6_ADDRS) &&
   151		    proto == ETH_P_IPV6) {
   152			return -EOPNOTSUPP;
   153		}
   154	
   155		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PORTS)) {
   156			struct flow_match_ports match;
   157	
   158			flow_rule_match_ports(rule, &match);
   159			ace->frame.ipv4.sport.value = ntohs(match.key->src);
   160			ace->frame.ipv4.sport.mask = ntohs(match.mask->src);
   161			ace->frame.ipv4.dport.value = ntohs(match.key->dst);
   162			ace->frame.ipv4.dport.mask = ntohs(match.mask->dst);
   163			match_protocol = false;
   164		}
   165	
   166		if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
   167			struct flow_match_vlan match;
   168	
   169			flow_rule_match_vlan(rule, &match);
   170			ace->type = OCELOT_ACE_TYPE_ANY;
   171			ace->vlan.vid.value = match.key->vlan_id;
   172			ace->vlan.vid.mask = match.mask->vlan_id;
   173			ace->vlan.pcp.value[0] = match.key->vlan_priority;
   174			ace->vlan.pcp.mask[0] = match.mask->vlan_priority;
   175			match_protocol = false;
   176		}
   177	
   178	finished_key_parsing:
   179		if (match_protocol && proto != ETH_P_ALL) {
   180			/* TODO: support SNAP, LLC etc */
   181			if (proto < ETH_P_802_3_MIN)
   182				return -EOPNOTSUPP;
   183			ace->type = OCELOT_ACE_TYPE_ETYPE;
 > 184			*(u16 *)ace->frame.etype.etype.value = htons(proto);
   185			*(u16 *)ace->frame.etype.etype.mask = 0xffff;
   186		}
   187		/* else, a rule of type OCELOT_ACE_TYPE_ANY is implicitly added */
   188	
   189		ace->prio = f->common.prio;
   190		ace->id = f->cookie;
   191		return ocelot_flower_parse_action(f, ace);
   192	}
   193	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[Vladimir Oltean]

On Thu, 23 Apr 2020 at 01:36, kbuild test robot <lkp@intel.com> wrote:
>
> Hi Vladimir,
>

[...]

>
> sparse warnings: (new ones prefixed by >>)
>
> >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [usertype] @@    got resunsigned short [usertype] @@

What's a resunsigned short?

> >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    expected unsigned short [usertype]
> >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    got restricted __be16 [usertype]
>

[...]

>    179          if (match_protocol && proto != ETH_P_ALL) {
>    180                  /* TODO: support SNAP, LLC etc */
>    181                  if (proto < ETH_P_802_3_MIN)
>    182                          return -EOPNOTSUPP;
>    183                  ace->type = OCELOT_ACE_TYPE_ETYPE;
>  > 184                  *(u16 *)ace->frame.etype.etype.value = htons(proto);

What's wrong with this? Doesn't it like the left hand side or the
right hand side?

>    185                  *(u16 *)ace->frame.etype.etype.mask = 0xffff;
>    186          }

>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

-Vladimir

Re: [PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[Marcelo Ricardo Leitner]

On Thu, Apr 23, 2020 at 01:50:41AM +0300, Vladimir Oltean wrote:
> On Thu, 23 Apr 2020 at 01:36, kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Vladimir,
> >
> 
> [...]
> 
> >
> > sparse warnings: (new ones prefixed by >>)
> >
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [usertype] @@    got resunsigned short [usertype] @@
> 
> What's a resunsigned short?

Dunno, but seems "restricted unsigned short", considering the below.

> 
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    expected unsigned short [usertype]
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    got restricted __be16 [usertype]
> >
> 
> [...]
> 
> >    179          if (match_protocol && proto != ETH_P_ALL) {
> >    180                  /* TODO: support SNAP, LLC etc */
> >    181                  if (proto < ETH_P_802_3_MIN)
> >    182                          return -EOPNOTSUPP;
> >    183                  ace->type = OCELOT_ACE_TYPE_ETYPE;
> >  > 184                  *(u16 *)ace->frame.etype.etype.value = htons(proto);
> 
> What's wrong with this? Doesn't it like the left hand side or the
> right hand side?

It's assigning a big-endian value (network endian) to a host-endian
variable (u16 cast).

> 
> >    185                  *(u16 *)ace->frame.etype.etype.mask = 0xffff;
> >    186          }
> 
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 
> -Vladimir
> 

Re: [PATCH net-next 1/3] net: mscc: ocelot: support matching on EtherType

[Marcelo Ricardo Leitner]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Thu, Apr 23, 2020 at 01:50:41AM +0300, Vladimir Oltean wrote:
> On Thu, 23 Apr 2020 at 01:36, kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Vladimir,
> >
> 
> [...]
> 
> >
> > sparse warnings: (new ones prefixed by >>)
> >
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [usertype] @@    got resunsigned short [usertype] @@
> 
> What's a resunsigned short?

Dunno, but seems "restricted unsigned short", considering the below.

> 
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    expected unsigned short [usertype]
> > >> drivers/net/ethernet/mscc/ocelot_flower.c:184:54: sparse:    got restricted __be16 [usertype]
> >
> 
> [...]
> 
> >    179          if (match_protocol && proto != ETH_P_ALL) {
> >    180                  /* TODO: support SNAP, LLC etc */
> >    181                  if (proto < ETH_P_802_3_MIN)
> >    182                          return -EOPNOTSUPP;
> >    183                  ace->type = OCELOT_ACE_TYPE_ETYPE;
> >  > 184                  *(u16 *)ace->frame.etype.etype.value = htons(proto);
> 
> What's wrong with this? Doesn't it like the left hand side or the
> right hand side?

It's assigning a big-endian value (network endian) to a host-endian
variable (u16 cast).

> 
> >    185                  *(u16 *)ace->frame.etype.etype.mask = 0xffff;
> >    186          }
> 
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
> 
> -Vladimir
> 

[PATCH net-next 2/3] net: mscc: ocelot: refine the ocelot_ace_is_problematic_mac_etype function

[Vladimir Oltean]

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

The commit mentioned below was a bit too harsh, and while it restricted
the invalid key combinations which are known to not work, such as:

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

it also restricted some which still should work, such as:

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

What actually does not match "sanely" is a MAC_ETYPE rule on frames
having an EtherType of ARP, IPv4, IPv6, in addition to SNAP and OAM
frames (which the ocelot tc-flower implementation does not parse yet, so
the function might need to be revisited again in the future).

So just make the function recognize the problematic MAC_ETYPE rules by
EtherType - thus the VCAP IS2 can be forced to match even on those
packets.

This patch makes it possible for IP rules to live on a port together
with MAC_ETYPE rules that are non-all, non-arp, non-ip and non-ipv6.

Fixes: d4d0cb741d7b ("net: mscc: ocelot: deal with problematic MAC_ETYPE VCAP IS2 rules")
Reported-by: Allan W. Nielsen <allan.nielsen@microchip.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_ace.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
index 8a2f7d13ef6d..dfd82a3baab2 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.c
+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
@@ -739,14 +739,24 @@ static void ocelot_match_all_as_mac_etype(struct ocelot *ocelot, int port,
 
 static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
 {
+	u16 proto, mask;
+
 	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))
+
+	proto = ntohs(*(u16 *)ace->frame.etype.etype.value);
+	mask = ntohs(*(u16 *)ace->frame.etype.etype.mask);
+
+	/* ETH_P_ALL match, so all protocols below are included */
+	if (mask == 0)
 		return true;
-	if (ether_addr_to_u64(ace->frame.etype.smac.value) &
-	    ether_addr_to_u64(ace->frame.etype.smac.mask))
+	if (proto == ETH_P_ARP)
 		return true;
+	if (proto == ETH_P_IP)
+		return true;
+	if (proto == ETH_P_IPV6)
+		return true;
+
 	return false;
 }
 
-- 
2.17.1

Re: [PATCH net-next 2/3] net: mscc: ocelot: refine the ocelot_ace_is_problematic_mac_etype function

[kbuild test robot]

Hi Vladimir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on next-20200421]
[cannot apply to net/master linus/master sparc-next/master v5.7-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Ocelot-MAC_ETYPE-tc-flower-key-improvements/20200422-113906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b6246f4d8d0778fd045b84dbd7fc5aadd8f3136e
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16

vim +747 drivers/net/ethernet/mscc/ocelot_ace.c

   739	
   740	static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
   741	{
   742		u16 proto, mask;
   743	
   744		if (ace->type != OCELOT_ACE_TYPE_ETYPE)
   745			return false;
   746	
 > 747		proto = ntohs(*(u16 *)ace->frame.etype.etype.value);
   748		mask = ntohs(*(u16 *)ace->frame.etype.etype.mask);
   749	
   750		/* ETH_P_ALL match, so all protocols below are included */
   751		if (mask == 0)
   752			return true;
   753		if (proto == ETH_P_ARP)
   754			return true;
   755		if (proto == ETH_P_IP)
   756			return true;
   757		if (proto == ETH_P_IPV6)
   758			return true;
   759	
   760		return false;
   761	}
   762	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH net-next 2/3] net: mscc: ocelot: refine the ocelot_ace_is_problematic_mac_etype function

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

Hi Vladimir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on next-20200421]
[cannot apply to net/master linus/master sparc-next/master v5.7-rc2]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Vladimir-Oltean/Ocelot-MAC_ETYPE-tc-flower-key-improvements/20200422-113906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b6246f4d8d0778fd045b84dbd7fc5aadd8f3136e
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
>> drivers/net/ethernet/mscc/ocelot_ace.c:747:17: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16
   drivers/net/ethernet/mscc/ocelot_ace.c:748:16: sparse: sparse: cast to restricted __be16

vim +747 drivers/net/ethernet/mscc/ocelot_ace.c

   739	
   740	static bool ocelot_ace_is_problematic_mac_etype(struct ocelot_ace_rule *ace)
   741	{
   742		u16 proto, mask;
   743	
   744		if (ace->type != OCELOT_ACE_TYPE_ETYPE)
   745			return false;
   746	
 > 747		proto = ntohs(*(u16 *)ace->frame.etype.etype.value);
   748		mask = ntohs(*(u16 *)ace->frame.etype.etype.mask);
   749	
   750		/* ETH_P_ALL match, so all protocols below are included */
   751		if (mask == 0)
   752			return true;
   753		if (proto == ETH_P_ARP)
   754			return true;
   755		if (proto == ETH_P_IP)
   756			return true;
   757		if (proto == ETH_P_IPV6)
   758			return true;
   759	
   760		return false;
   761	}
   762	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH net-next 3/3] net: mscc: ocelot: lift protocol restriction for flow_match_eth_addrs keys

[Vladimir Oltean]

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

An attempt was made in commit fe3490e6107e ("net: mscc: ocelot: Hardware
ofload for tc flower filter") to avoid clashes between MAC_ETYPE rules
and IP rules. Because the protocol blacklist should have included
ETH_P_ALL too, it created some confusion, but now the situation should
be dealt with a bit better by the patch immediately previous to this one
("net: mscc: ocelot: refine the ocelot_ace_is_problematic_mac_etype
function").

So now we can remove that check. MAC_ETYPE rules with a protocol of
ETH_P_IP, ETH_P_IPV6, ETH_P_ARP and ETH_P_ALL _are_ supported, with some
restrictions regarding per-port exclusivity which are enforced now.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 67f0f5455ff0..5ce172e22b43 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -87,11 +87,6 @@ static int ocelot_flower_parse(struct flow_cls_offload *f,
 		     BIT(FLOW_DISSECTOR_KEY_CONTROL)))
 			return -EOPNOTSUPP;
 
-		if (proto == ETH_P_IP ||
-		    proto == ETH_P_IPV6 ||
-		    proto == ETH_P_ARP)
-			return -EOPNOTSUPP;
-
 		flow_rule_match_eth_addrs(rule, &match);
 		ace->type = OCELOT_ACE_TYPE_ETYPE;
 		ether_addr_copy(ace->frame.etype.dmac.value,
-- 
2.17.1

Re: [PATCH net-next 0/3] Ocelot MAC_ETYPE tc-flower key improvements

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon, 20 Apr 2020 19:27:40 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> As discussed in the comments surrounding this patch:
> https://patchwork.ozlabs.org/project/netdev/patch/20200417190308.32598-1-olteanv@gmail.com/
> 
> the restrictions imposed on non-MAC_ETYPE rules were harsher than they
> needed to be. IP, IPv6, ARP rules can still be added concurrently with
> src_mac and dst_mac rules, as long as those MAC address rules do not ask
> for an offending EtherType.
> 
> For that to actually be supported, we need to parse the EtherType from
> the flower classification rule first.

Series applied, thanks.