| Message ID | 20201110075355.52075-1-zahari.doychev@linux.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v2] tc flower: use right ethertype in icmp/arp parsing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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
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
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
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); + ð_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; }
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(-)