[PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[Zahari Doychev]

Currently the icmp and arp parsing functions are called with incorrect
ethtype in case of vlan or cvlan filter options. In this case either
cvlan_ethtype or vlan_ethtype has to be used. The ethtype is now updated
each time a vlan ethtype is matched during parsing.

Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
 tc/f_flower.c | 52 +++++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/tc/f_flower.c b/tc/f_flower.c
index 00c919fd..58e1140d 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -1324,9 +1324,9 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 	bool mpls_format_old = false;
 	bool mpls_format_new = false;
 	struct rtattr *tail;
-	__be16 eth_type = TC_H_MIN(t->tcm_info);
+	__be16 tc_proto = TC_H_MIN(t->tcm_info);
+	__be16 eth_type = tc_proto;
 	__be16 vlan_ethtype = 0;
-	__be16 cvlan_ethtype = 0;
 	__u8 ip_proto = 0xff;
 	__u32 flags = 0;
 	__u32 mtf = 0;
@@ -1464,6 +1464,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 						 &vlan_ethtype, n);
 			if (ret < 0)
 				return -1;
+			/* get new ethtype for later parsing  */
+			eth_type = vlan_ethtype;
 		} else if (matches(*argv, "cvlan_id") == 0) {
 			__u16 vid;
 
@@ -1495,9 +1497,10 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 				 TCA_FLOWER_KEY_CVLAN_PRIO, cvlan_prio);
 		} else if (matches(*argv, "cvlan_ethtype") == 0) {
 			NEXT_ARG();
+			/* get new ethtype for later parsing */
 			ret = flower_parse_vlan_eth_type(*argv, vlan_ethtype,
 						 TCA_FLOWER_KEY_CVLAN_ETH_TYPE,
-						 &cvlan_ethtype, n);
+						 &eth_type, n);
 			if (ret < 0)
 				return -1;
 		} else if (matches(*argv, "mpls") == 0) {
@@ -1627,9 +1630,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 			}
 		} else if (matches(*argv, "ip_proto") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_ip_proto(*argv, cvlan_ethtype ?
-						    cvlan_ethtype : vlan_ethtype ?
-						    vlan_ethtype : eth_type,
+			ret = flower_parse_ip_proto(*argv, eth_type,
 						    TCA_FLOWER_KEY_IP_PROTO,
 						    &ip_proto, n);
 			if (ret < 0) {
@@ -1658,9 +1659,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 			}
 		} else if (matches(*argv, "dst_ip") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_ip_addr(*argv, cvlan_ethtype ?
-						   cvlan_ethtype : vlan_ethtype ?
-						   vlan_ethtype : eth_type,
+			ret = flower_parse_ip_addr(*argv, eth_type,
 						   TCA_FLOWER_KEY_IPV4_DST,
 						   TCA_FLOWER_KEY_IPV4_DST_MASK,
 						   TCA_FLOWER_KEY_IPV6_DST,
@@ -1672,9 +1671,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 			}
 		} else if (matches(*argv, "src_ip") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_ip_addr(*argv, cvlan_ethtype ?
-						   cvlan_ethtype : vlan_ethtype ?
-						   vlan_ethtype : eth_type,
+			ret = flower_parse_ip_addr(*argv, eth_type,
 						   TCA_FLOWER_KEY_IPV4_SRC,
 						   TCA_FLOWER_KEY_IPV4_SRC_MASK,
 						   TCA_FLOWER_KEY_IPV6_SRC,
@@ -1728,33 +1725,30 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 			}
 		} else if (matches(*argv, "arp_tip") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_arp_ip_addr(*argv, vlan_ethtype ?
-						       vlan_ethtype : eth_type,
-						       TCA_FLOWER_KEY_ARP_TIP,
-						       TCA_FLOWER_KEY_ARP_TIP_MASK,
-						       n);
+			ret = flower_parse_arp_ip_addr(*argv, eth_type,
+						TCA_FLOWER_KEY_ARP_TIP,
+						TCA_FLOWER_KEY_ARP_TIP_MASK,
+						n);
 			if (ret < 0) {
 				fprintf(stderr, "Illegal \"arp_tip\"\n");
 				return -1;
 			}
 		} else if (matches(*argv, "arp_sip") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_arp_ip_addr(*argv, vlan_ethtype ?
-						       vlan_ethtype : eth_type,
-						       TCA_FLOWER_KEY_ARP_SIP,
-						       TCA_FLOWER_KEY_ARP_SIP_MASK,
-						       n);
+			ret = flower_parse_arp_ip_addr(*argv, eth_type,
+						TCA_FLOWER_KEY_ARP_SIP,
+						TCA_FLOWER_KEY_ARP_SIP_MASK,
+						n);
 			if (ret < 0) {
 				fprintf(stderr, "Illegal \"arp_sip\"\n");
 				return -1;
 			}
 		} else if (matches(*argv, "arp_op") == 0) {
 			NEXT_ARG();
-			ret = flower_parse_arp_op(*argv, vlan_ethtype ?
-						  vlan_ethtype : eth_type,
-						  TCA_FLOWER_KEY_ARP_OP,
-						  TCA_FLOWER_KEY_ARP_OP_MASK,
-						  n);
+			ret = flower_parse_arp_op(*argv, eth_type,
+						TCA_FLOWER_KEY_ARP_OP,
+						TCA_FLOWER_KEY_ARP_OP_MASK,
+						n);
 			if (ret < 0) {
 				fprintf(stderr, "Illegal \"arp_op\"\n");
 				return -1;
@@ -1894,8 +1888,8 @@ parse_done:
 			return ret;
 	}
 
-	if (eth_type != htons(ETH_P_ALL)) {
-		ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, eth_type);
+	if (tc_proto != htons(ETH_P_ALL)) {
+		ret = addattr16(n, MAX_MSG, TCA_FLOWER_KEY_ETH_TYPE, tc_proto);
 		if (ret)
 			return ret;
 	}
-- 
2.28.0

Re: [PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[David Ahern]

On 11/10/20 12:53 AM, Zahari Doychev wrote:
> Currently the icmp and arp parsing functions are called with incorrect
> ethtype in case of vlan or cvlan filter options. In this case either
> cvlan_ethtype or vlan_ethtype has to be used. The ethtype is now updated
> each time a vlan ethtype is matched during parsing.
> 
> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> ---
>  tc/f_flower.c | 52 +++++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 29 deletions(-)
> 

Thanks, looks much better.

applied to iproute2-next.

Re: [PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[Roi Dayan]

On 2020-11-14 5:12 AM, David Ahern wrote:
> On 11/10/20 12:53 AM, Zahari Doychev wrote:
>> Currently the icmp and arp parsing functions are called with incorrect
>> ethtype in case of vlan or cvlan filter options. In this case either
>> cvlan_ethtype or vlan_ethtype has to be used. The ethtype is now updated
>> each time a vlan ethtype is matched during parsing.
>>
>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
>> ---
>>   tc/f_flower.c | 52 +++++++++++++++++++++++----------------------------
>>   1 file changed, 23 insertions(+), 29 deletions(-)
>>
> 
> Thanks, looks much better.
> 
> applied to iproute2-next.
> 

Hi,

I didn't debug yet but with this commit I am failing to add a tc
rule I always could before. also the error msg doesn't make sense.

Example:

# tc filter add dev enp8s0f0 protocol 802.1Q parent ffff: prio 1 flower\
  skip_hw dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50\
  vlan_ethtype 0x800 vlan_id 100 vlan_prio 0 action vlan pop action\
  mirred egress redirect dev enp8s0f0_0


Can't set "vlan_id" if ethertype isn't 802.1Q or 802.1AD


I used protocol 802.1Q and vlan_ethtype 0x800.
am i missing something? the rule should look different now?

Thanks,
Roi

Re: [PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[Roi Dayan]

On 2020-11-24 11:39 AM, Roi Dayan wrote:
> 
> 
> On 2020-11-14 5:12 AM, David Ahern wrote:
>> On 11/10/20 12:53 AM, Zahari Doychev wrote:
>>> Currently the icmp and arp parsing functions are called with incorrect
>>> ethtype in case of vlan or cvlan filter options. In this case either
>>> cvlan_ethtype or vlan_ethtype has to be used. The ethtype is now updated
>>> each time a vlan ethtype is matched during parsing.
>>>
>>> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
>>> ---
>>>   tc/f_flower.c | 52 +++++++++++++++++++++++----------------------------
>>>   1 file changed, 23 insertions(+), 29 deletions(-)
>>>
>>
>> Thanks, looks much better.
>>
>> applied to iproute2-next.
>>
> 
> Hi,
> 
> I didn't debug yet but with this commit I am failing to add a tc
> rule I always could before. also the error msg doesn't make sense.
> 
> Example:
> 
> # tc filter add dev enp8s0f0 protocol 802.1Q parent ffff: prio 1 flower\
>   skip_hw dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50\
>   vlan_ethtype 0x800 vlan_id 100 vlan_prio 0 action vlan pop action\
>   mirred egress redirect dev enp8s0f0_0
> 
> 
> Can't set "vlan_id" if ethertype isn't 802.1Q or 802.1AD
> 
> 
> I used protocol 802.1Q and vlan_ethtype 0x800.
> am i missing something? the rule should look different now?
> 
> Thanks,
> Roi


Hi,

I debugged this and it break vlan rules.
The issue is from this part of the change


@@ -1464,6 +1464,8 @@ static int flower_parse_opt(struct filter_util 
*qu, char *handle,
                                                  &vlan_ethtype, n);
                         if (ret < 0)
                                 return -1;
+                       /* get new ethtype for later parsing  */
+                       eth_type = vlan_ethtype;



Now params vlan_id, vlan_prio check if eth_type is vlan but it's not.
it's 0x0800 as the example as it was set to the vlan_ethtype.

Need to continue check the outer, so tc_proto.
i'll prep a fix commit for review.


Thanks,
Roi

Re: [PATCH iproute2-next v2] tc flower: use right ethertype in icmp/arp parsing

[Jamal Hadi Salim]

On 2020-11-24 7:13 a.m., Roi Dayan wrote:
> 
> 
> On 2020-11-24 11:39 AM, Roi Dayan wrote:
>>
>>

[..]
>> Hi,
>>
>> I didn't debug yet but with this commit I am failing to add a tc
>> rule I always could before. also the error msg doesn't make sense.
>>
>> Example:
>>
>> # tc filter add dev enp8s0f0 protocol 802.1Q parent ffff: prio 1 flower\
>>   skip_hw dst_mac e4:11:22:11:4a:51 src_mac e4:11:22:11:4a:50\
>>   vlan_ethtype 0x800 vlan_id 100 vlan_prio 0 action vlan pop action\
>>   mirred egress redirect dev enp8s0f0_0
>>
>>
>> Can't set "vlan_id" if ethertype isn't 802.1Q or 802.1AD
>>
>>
>> I used protocol 802.1Q and vlan_ethtype 0x800.
>> am i missing something? the rule should look different now?
>>
>> Thanks,
>> Roi
> 
> 
> Hi,
> 
> I debugged this and it break vlan rules.
> The issue is from this part of the change
> 

We have a test for this in tdc. Maybe we should make it a rule to
run tdc tests on changes like these?

cheers,
jamal