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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190725034540.GJ6204@localhost.localdomain>
Date:   Thu, 25 Jul 2019 00:45:40 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     wenxu <wenxu@...oud.cn>
Cc:     pablo@...filter.org, davem@...emloft.net,
        netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] netfilter: nf_table_offload: Fix zero prio of
 flow_cls_common_offload

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@...oud.cn wrote:
> >> From: wenxu <wenxu@...oud.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@...oud.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