Re: [PATCH net-next] netfilter: nf_table_offload: Fix zero prio of flow_cls_common_offload

[wenxu]

On 7/25/2019 7:51 AM, Marcelo Ricardo Leitner wrote:
> On Thu, Jul 11, 2019 at 04:03:30PM +0800, we...@ucloud.cn wrote:
>> From: wenxu <we...@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 <we...@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);
}