| Message ID | 1562832210-25981-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net-next] netfilter: nf_table_offload: Fix zero prio of flow_cls_common_offload | expand |
On Thu, Jul 11, 2019 at 04:03:30PM +0800, wenxu@ucloud.cn wrote: > From: wenxu <wenxu@ucloud.cn> > > The flow_cls_common_offload prio should be not zero > > It leads the invalid table prio in hw. > > # nft add table netdev firewall > # nft add chain netdev firewall acl { type filter hook ingress device mlx_pf0vf0 priority - 300 \; } > # nft add rule netdev firewall acl ip daddr 1.1.1.7 drop > Error: Could not process rule: Invalid argument > > kernel log > mlx5_core 0000:81:00.0: E-Switch: Failed to create FDB Table err -22 (table prio: 65535, level: 0, size: 4194304) > > Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") > Signed-off-by: wenxu <wenxu@ucloud.cn> > --- > net/netfilter/nf_tables_offload.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c > index 2c33028..01d8133 100644 > --- a/net/netfilter/nf_tables_offload.c > +++ b/net/netfilter/nf_tables_offload.c > @@ -7,6 +7,8 @@ > #include <net/netfilter/nf_tables_offload.h> > #include <net/pkt_cls.h> > > +#define FLOW_OFFLOAD_DEFAUT_PRIO 1U > + > static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) > { > struct nft_flow_rule *flow; > @@ -107,6 +109,7 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common, > struct netlink_ext_ack *extack) > { > common->protocol = proto; > + common->prio = TC_H_MAKE(FLOW_OFFLOAD_DEFAUT_PRIO << 16, 0); Note that tc semantics for this is to auto-generate a priority in such cases, instead of using a default. @tc_new_tfilter(): if (prio == 0) { /* If no priority is provided by the user, * we allocate one. */ if (n->nlmsg_flags & NLM_F_CREATE) { prio = TC_H_MAKE(0x80000000U, 0U); prio_allocate = true; ... if (prio_allocate) prio = tcf_auto_prio(tcf_chain_tp_prev(chain, &chain_info)); > common->extack = extack; > } > > -- > 1.8.3.1 >
On 7/25/2019 7:51 AM, Marcelo Ricardo Leitner wrote: > On Thu, Jul 11, 2019 at 04:03:30PM +0800, wenxu@ucloud.cn wrote: >> From: wenxu <wenxu@ucloud.cn> >> >> The flow_cls_common_offload prio should be not zero >> >> It leads the invalid table prio in hw. >> >> # nft add table netdev firewall >> # nft add chain netdev firewall acl { type filter hook ingress device mlx_pf0vf0 priority - 300 \; } >> # nft add rule netdev firewall acl ip daddr 1.1.1.7 drop >> Error: Could not process rule: Invalid argument >> >> kernel log >> mlx5_core 0000:81:00.0: E-Switch: Failed to create FDB Table err -22 (table prio: 65535, level: 0, size: 4194304) >> >> Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> --- >> net/netfilter/nf_tables_offload.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c >> index 2c33028..01d8133 100644 >> --- a/net/netfilter/nf_tables_offload.c >> +++ b/net/netfilter/nf_tables_offload.c >> @@ -7,6 +7,8 @@ >> #include <net/netfilter/nf_tables_offload.h> >> #include <net/pkt_cls.h> >> >> +#define FLOW_OFFLOAD_DEFAUT_PRIO 1U >> + >> static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) >> { >> struct nft_flow_rule *flow; >> @@ -107,6 +109,7 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common, >> struct netlink_ext_ack *extack) >> { >> common->protocol = proto; >> + common->prio = TC_H_MAKE(FLOW_OFFLOAD_DEFAUT_PRIO << 16, 0); > Note that tc semantics for this is to auto-generate a priority in such > cases, instead of using a default. > > @tc_new_tfilter(): > if (prio == 0) { > /* If no priority is provided by the user, > * we allocate one. > */ > if (n->nlmsg_flags & NLM_F_CREATE) { > prio = TC_H_MAKE(0x80000000U, 0U); > prio_allocate = true; > ... > if (prio_allocate) > prio = tcf_auto_prio(tcf_chain_tp_prev(chain, > &chain_info)); Yes,The tc auto-generate a priority. But if there is no pre tcf_proto, the priority is also set as a default. In nftables each rule no priortiy for each other. So It is enough to set a default value which is similar as the tc. static inline u32 tcf_auto_prio(struct tcf_proto *tp) { u32 first = TC_H_MAKE(0xC0000000U, 0U); if (tp) first = tp->prio - 1; return TC_H_MAJ(first); }
On Thu, Jul 25, 2019 at 11:03:52AM +0800, wenxu wrote: > > On 7/25/2019 7:51 AM, Marcelo Ricardo Leitner wrote: > > On Thu, Jul 11, 2019 at 04:03:30PM +0800, wenxu@ucloud.cn wrote: > >> From: wenxu <wenxu@ucloud.cn> > >> > >> The flow_cls_common_offload prio should be not zero > >> > >> It leads the invalid table prio in hw. > >> > >> # nft add table netdev firewall > >> # nft add chain netdev firewall acl { type filter hook ingress device mlx_pf0vf0 priority - 300 \; } > >> # nft add rule netdev firewall acl ip daddr 1.1.1.7 drop > >> Error: Could not process rule: Invalid argument > >> > >> kernel log > >> mlx5_core 0000:81:00.0: E-Switch: Failed to create FDB Table err -22 (table prio: 65535, level: 0, size: 4194304) > >> > >> Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") > >> Signed-off-by: wenxu <wenxu@ucloud.cn> > >> --- > >> net/netfilter/nf_tables_offload.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c > >> index 2c33028..01d8133 100644 > >> --- a/net/netfilter/nf_tables_offload.c > >> +++ b/net/netfilter/nf_tables_offload.c > >> @@ -7,6 +7,8 @@ > >> #include <net/netfilter/nf_tables_offload.h> > >> #include <net/pkt_cls.h> > >> > >> +#define FLOW_OFFLOAD_DEFAUT_PRIO 1U > >> + > >> static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) > >> { > >> struct nft_flow_rule *flow; > >> @@ -107,6 +109,7 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common, > >> struct netlink_ext_ack *extack) > >> { > >> common->protocol = proto; > >> + common->prio = TC_H_MAKE(FLOW_OFFLOAD_DEFAUT_PRIO << 16, 0); > > Note that tc semantics for this is to auto-generate a priority in such > > cases, instead of using a default. > > > > @tc_new_tfilter(): > > if (prio == 0) { > > /* If no priority is provided by the user, > > * we allocate one. > > */ > > if (n->nlmsg_flags & NLM_F_CREATE) { > > prio = TC_H_MAKE(0x80000000U, 0U); > > prio_allocate = true; > > ... > > if (prio_allocate) > > prio = tcf_auto_prio(tcf_chain_tp_prev(chain, > > &chain_info)); > > Yes,The tc auto-generate a priority. But if there is no pre > tcf_proto, the priority is also set as a default. After the first filter, there will be a tcf_proto. Please see the test below. > > In nftables each rule no priortiy for each other. So It is enough to > set a default value which is similar as the tc. Yep, maybe it works for nftables. I'm just highlighting this because it is reusing tc infrastructure and will expose a different behavior to the user. But if nftables already has this defined, that probably takes precedence by now and all that is left to do is to make sure any documentation on it is updated. Pablo? > > static inline u32 tcf_auto_prio(struct tcf_proto *tp) > { > u32 first = TC_H_MAKE(0xC0000000U, 0U); ^^^^ base default prio, 0xC0000 = 49152 > > if (tp) > first = tp->prio - 1; > > return TC_H_MAJ(first); > } # tc qdisc add dev veth1 ingress # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:00 action drop 1st filter --^^ # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:01 action drop 2nd filter --^^ # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:02 action drop With no 'prio X' parameter, it uses 0 as default, and when dumped: # tc filter show dev veth1 ingress filter protocol ip pref 49150 flower filter protocol ip pref 49150 flower handle 0x1 src_mac ec:13:db:00:00:02 eth_type ipv4 not_in_hw action order 1: gact action drop random type none pass val 0 index 40003 ref 1 bind 1 filter protocol ip pref 49151 flower filter protocol ip pref 49151 flower handle 0x1 ^vv^^---- 2nd filter src_mac ec:13:db:00:00:01 eth_type ipv4 not_in_hw action order 1: gact action drop random type none pass val 0 index 40002 ref 1 bind 1 filter protocol ip pref 49152 flower filter protocol ip pref 49152 flower handle 0x1 ^vv^^---- 1st filter src_mac ec:13:db:00:00:00 eth_type ipv4 not_in_hw action order 1: gact action drop random type none pass val 0 index 40001 ref 1 bind 1
Hi pablo Any suggestion for this case. Tthe 0 prio vlaue for driver is an invalid priority. So What should we do for this case? Currently there is no prio for each nft rules. BR wenxu On 7/25/2019 11:45 AM, Marcelo Ricardo Leitner wrote: > On Thu, Jul 25, 2019 at 11:03:52AM +0800, wenxu wrote: >> On 7/25/2019 7:51 AM, Marcelo Ricardo Leitner wrote: >>> On Thu, Jul 11, 2019 at 04:03:30PM +0800, wenxu@ucloud.cn wrote: >>>> From: wenxu <wenxu@ucloud.cn> >>>> >>>> The flow_cls_common_offload prio should be not zero >>>> >>>> It leads the invalid table prio in hw. >>>> >>>> # nft add table netdev firewall >>>> # nft add chain netdev firewall acl { type filter hook ingress device mlx_pf0vf0 priority - 300 \; } >>>> # nft add rule netdev firewall acl ip daddr 1.1.1.7 drop >>>> Error: Could not process rule: Invalid argument >>>> >>>> kernel log >>>> mlx5_core 0000:81:00.0: E-Switch: Failed to create FDB Table err -22 (table prio: 65535, level: 0, size: 4194304) >>>> >>>> Fixes: c9626a2cbdb2 ("netfilter: nf_tables: add hardware offload support") >>>> Signed-off-by: wenxu <wenxu@ucloud.cn> >>>> --- >>>> net/netfilter/nf_tables_offload.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c >>>> index 2c33028..01d8133 100644 >>>> --- a/net/netfilter/nf_tables_offload.c >>>> +++ b/net/netfilter/nf_tables_offload.c >>>> @@ -7,6 +7,8 @@ >>>> #include <net/netfilter/nf_tables_offload.h> >>>> #include <net/pkt_cls.h> >>>> >>>> +#define FLOW_OFFLOAD_DEFAUT_PRIO 1U >>>> + >>>> static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) >>>> { >>>> struct nft_flow_rule *flow; >>>> @@ -107,6 +109,7 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common, >>>> struct netlink_ext_ack *extack) >>>> { >>>> common->protocol = proto; >>>> + common->prio = TC_H_MAKE(FLOW_OFFLOAD_DEFAUT_PRIO << 16, 0); >>> Note that tc semantics for this is to auto-generate a priority in such >>> cases, instead of using a default. >>> >>> @tc_new_tfilter(): >>> if (prio == 0) { >>> /* If no priority is provided by the user, >>> * we allocate one. >>> */ >>> if (n->nlmsg_flags & NLM_F_CREATE) { >>> prio = TC_H_MAKE(0x80000000U, 0U); >>> prio_allocate = true; >>> ... >>> if (prio_allocate) >>> prio = tcf_auto_prio(tcf_chain_tp_prev(chain, >>> &chain_info)); >> Yes,The tc auto-generate a priority. But if there is no pre >> tcf_proto, the priority is also set as a default. > After the first filter, there will be a tcf_proto. Please see the test below. > >> In nftables each rule no priortiy for each other. So It is enough to >> set a default value which is similar as the tc. > Yep, maybe it works for nftables. I'm just highlighting this because > it is reusing tc infrastructure and will expose a different behavior > to the user. But if nftables already has this defined, that probably > takes precedence by now and all that is left to do is to make sure any > documentation on it is updated. Pablo? > >> static inline u32 tcf_auto_prio(struct tcf_proto *tp) >> { >> u32 first = TC_H_MAKE(0xC0000000U, 0U); > ^^^^ base default prio, 0xC0000 = 49152 > >> if (tp) >> first = tp->prio - 1; >> >> return TC_H_MAJ(first); >> } > # tc qdisc add dev veth1 ingress > # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:00 action drop > 1st filter --^^ > # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:01 action drop > 2nd filter --^^ > # tc filter add dev veth1 ingress proto ip flower src_mac ec:13:db:00:00:02 action drop > > With no 'prio X' parameter, it uses 0 as default, and when dumped: > > # tc filter show dev veth1 ingress > filter protocol ip pref 49150 flower > filter protocol ip pref 49150 flower handle 0x1 > src_mac ec:13:db:00:00:02 > eth_type ipv4 > not_in_hw > action order 1: gact action drop > random type none pass val 0 > index 40003 ref 1 bind 1 > > filter protocol ip pref 49151 flower > filter protocol ip pref 49151 flower handle 0x1 > ^vv^^---- 2nd filter > src_mac ec:13:db:00:00:01 > eth_type ipv4 > not_in_hw > action order 1: gact action drop > random type none pass val 0 > index 40002 ref 1 bind 1 > > filter protocol ip pref 49152 flower > filter protocol ip pref 49152 flower handle 0x1 > ^vv^^---- 1st filter > src_mac ec:13:db:00:00:00 > eth_type ipv4 > not_in_hw > action order 1: gact action drop > random type none pass val 0 > index 40001 ref 1 bind 1 > > >
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c index 2c33028..01d8133 100644 --- a/net/netfilter/nf_tables_offload.c +++ b/net/netfilter/nf_tables_offload.c @@ -7,6 +7,8 @@ #include <net/netfilter/nf_tables_offload.h> #include <net/pkt_cls.h> +#define FLOW_OFFLOAD_DEFAUT_PRIO 1U + static struct nft_flow_rule *nft_flow_rule_alloc(int num_actions) { struct nft_flow_rule *flow; @@ -107,6 +109,7 @@ static void nft_flow_offload_common_init(struct flow_cls_common_offload *common, struct netlink_ext_ack *extack) { common->protocol = proto; + common->prio = TC_H_MAKE(FLOW_OFFLOAD_DEFAUT_PRIO << 16, 0); common->extack = extack; }