[net-next] netfilter: nf_table_offload: Fix zero prio of flow_cls_common_offload
diff mbox series

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
Related show

Commit Message

wenxu July 11, 2019, 8:03 a.m. UTC
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(+)

Comments

Marcelo Ricardo Leitner July 24, 2019, 11:51 p.m. UTC | #1
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
>
wenxu July 25, 2019, 3:03 a.m. UTC | #2
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);
}
Marcelo Ricardo Leitner July 25, 2019, 3:45 a.m. UTC | #3
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
wenxu July 29, 2019, 3:43 a.m. UTC | #4
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
>
>
>

Patch
diff mbox series

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;
 }