[PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[Pablo Neira Ayuso]

This patch maps basechain netfilter priorities from -8192 to 8191 to
hardware priority 0xC000 + 1. tcf_auto_prio() uses 0xC000 if the user
specifies no priority, then it subtract 1 for each new tcf_proto object.
This patch uses the hardware priority range from 0xC000 to 0xFFFF for
netfilter.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
This follows a rather conservative approach, I could just expose the
2^16 hardware priority range, but we may need to split this priority
range among the ethtool_rx, tc and netfilter subsystems to start with
and it should be possible to extend the priority range later on.

By netfilter priority, I'm refering to the basechain priority:

	add chain x y { type filter hook ingress device eth0 priority 0; }
                                                             ^^^^^^^^^^^

This is no transparently mapped to hardware, this patch shifts it to
make it fit into the 0xC000 + 1 .. 0xFFFF hardware priority range.

 include/net/netfilter/nf_tables_offload.h |  2 ++
 net/netfilter/nf_tables_api.c             |  4 ++++
 net/netfilter/nf_tables_offload.c         | 32 ++++++++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_tables_offload.h b/include/net/netfilter/nf_tables_offload.h
index 3196663a10e3..2d497394021e 100644
--- a/include/net/netfilter/nf_tables_offload.h
+++ b/include/net/netfilter/nf_tables_offload.h
@@ -73,4 +73,6 @@ int nft_flow_rule_offload_commit(struct net *net);
 	(__reg)->key		= __key;				\
 	memset(&(__reg)->mask, 0xff, (__reg)->len);
 
+u16 nft_chain_offload_priority(struct nft_base_chain *basechain);
+
 #endif
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 93647fdf435c..9ee6db9a668d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1669,6 +1669,10 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 genmask,
 
 		chain->flags |= NFT_BASE_CHAIN | flags;
 		basechain->policy = NF_ACCEPT;
+		if (chain->flags & NFT_CHAIN_HW_OFFLOAD &&
+		    !nft_chain_offload_priority(basechain))
+			return -EOPNOTSUPP;
+
 		flow_block_init(&basechain->flow_block);
 	} else {
 		chain = kzalloc(sizeof(*chain), GFP_KERNEL);
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index ec70978eba5a..df8427ba857c 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -156,10 +156,11 @@ void nft_offload_update_dependency(struct nft_offload_ctx *ctx,
 }
 
 static void nft_flow_offload_common_init(struct flow_cls_common_offload *common,
-					 __be16 proto,
-					struct netlink_ext_ack *extack)
+					 __be16 proto, int priority,
+					 struct netlink_ext_ack *extack)
 {
 	common->protocol = proto;
+	common->prio = priority;
 	common->extack = extack;
 }
 
@@ -180,6 +181,29 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
 	return 0;
 }
 
+/* Available priorities for hardware offload range: -8192..8191 */
+#define NFT_BASECHAIN_OFFLOAD_PRIO_MAX		(SHRT_MAX / 4)
+#define NFT_BASECHAIN_OFFLOAD_PRIO_MIN		(SHRT_MIN / 4)
+#define NFT_BASECHAIN_OFFLOAD_PRIO_RANGE	(USHRT_MAX / 4)
+/* tcf_auto_prio() uses 0xC000 as base, then subtract one for each new chain. */
+#define NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE	(0xC000 + 1)
+
+u16 nft_chain_offload_priority(struct nft_base_chain *basechain)
+{
+	u16 prio;
+
+	if (basechain->ops.priority < NFT_BASECHAIN_OFFLOAD_PRIO_MIN ||
+	    basechain->ops.priority > NFT_BASECHAIN_OFFLOAD_PRIO_MAX)
+		return 0;
+
+	/* map netfilter chain priority to hardware priority. */
+	prio = basechain->ops.priority +
+		NFT_BASECHAIN_OFFLOAD_PRIO_MAX +
+			NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE;
+
+	return prio;
+}
+
 static int nft_flow_offload_rule(struct nft_trans *trans,
 				 enum flow_cls_command command)
 {
@@ -200,7 +224,9 @@ static int nft_flow_offload_rule(struct nft_trans *trans,
 	if (flow)
 		proto = flow->proto;
 
-	nft_flow_offload_common_init(&cls_flow.common, proto, &extack);
+	nft_flow_offload_common_init(&cls_flow.common, proto,
+				     nft_chain_offload_priority(basechain),
+				     &extack);
 	cls_flow.command = command;
 	cls_flow.cookie = (unsigned long) rule;
 	if (flow)
-- 
2.11.0

Re: [PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[Pablo Neira Ayuso]

On Tue, Jul 30, 2019 at 12:54:17PM +0200, Pablo Neira Ayuso wrote:
[...]
> @@ -180,6 +181,29 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
>  	return 0;
>  }
>  
> +/* Available priorities for hardware offload range: -8192..8191 */
> +#define NFT_BASECHAIN_OFFLOAD_PRIO_MAX		(SHRT_MAX / 4)
> +#define NFT_BASECHAIN_OFFLOAD_PRIO_MIN		(SHRT_MIN / 4)
> +#define NFT_BASECHAIN_OFFLOAD_PRIO_RANGE	(USHRT_MAX / 4)
> +/* tcf_auto_prio() uses 0xC000 as base, then subtract one for each new chain. */
> +#define NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE	(0xC000 + 1)
> +
> +u16 nft_chain_offload_priority(struct nft_base_chain *basechain)
> +{
> +	u16 prio;
> +
> +	if (basechain->ops.priority < NFT_BASECHAIN_OFFLOAD_PRIO_MIN ||
> +	    basechain->ops.priority > NFT_BASECHAIN_OFFLOAD_PRIO_MAX)
> +		return 0;
> +
> +	/* map netfilter chain priority to hardware priority. */
> +	prio = basechain->ops.priority +
> +		NFT_BASECHAIN_OFFLOAD_PRIO_MAX +
> +			NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE;
> +
> +	return prio;

This function should actually return:

        return prio << 16;

> +}
> +
>  static int nft_flow_offload_rule(struct nft_trans *trans,
>  				 enum flow_cls_command command)
>  {

Re: [PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[Pablo Neira Ayuso]

On Tue, Jul 30, 2019 at 01:18:00PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 30, 2019 at 12:54:17PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > @@ -180,6 +181,29 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
> >  	return 0;
> >  }
> >  
> > +/* Available priorities for hardware offload range: -8192..8191 */
> > +#define NFT_BASECHAIN_OFFLOAD_PRIO_MAX		(SHRT_MAX / 4)
> > +#define NFT_BASECHAIN_OFFLOAD_PRIO_MIN		(SHRT_MIN / 4)
> > +#define NFT_BASECHAIN_OFFLOAD_PRIO_RANGE	(USHRT_MAX / 4)
> > +/* tcf_auto_prio() uses 0xC000 as base, then subtract one for each new chain. */
> > +#define NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE	(0xC000 + 1)
> > +
> > +u16 nft_chain_offload_priority(struct nft_base_chain *basechain)
> > +{
> > +	u16 prio;
> > +
> > +	if (basechain->ops.priority < NFT_BASECHAIN_OFFLOAD_PRIO_MIN ||
> > +	    basechain->ops.priority > NFT_BASECHAIN_OFFLOAD_PRIO_MAX)
> > +		return 0;
> > +
> > +	/* map netfilter chain priority to hardware priority. */
> > +	prio = basechain->ops.priority +
> > +		NFT_BASECHAIN_OFFLOAD_PRIO_MAX +
> > +			NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE;
> > +
> > +	return prio;
> 
> This function should actually return:
> 
>         return prio << 16;

Better shift it from here:

static void nft_flow_offload_common_init(struct flow_cls_common_offload *common,
                                         __be16 proto, int priority,
                                         struct netlink_ext_ack *extack)
{
        common->protocol = proto;
        common->prio = priority << 16;
        common->extack = extack;
}

Drivers assume tc handle format.

Re: [PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[Marcelo Ricardo Leitner]

On Tue, Jul 30, 2019 at 12:54:17PM +0200, Pablo Neira Ayuso wrote:
> This patch maps basechain netfilter priorities from -8192 to 8191 to
> hardware priority 0xC000 + 1. tcf_auto_prio() uses 0xC000 if the user
> specifies no priority, then it subtract 1 for each new tcf_proto object.
> This patch uses the hardware priority range from 0xC000 to 0xFFFF for
> netfilter.

This makes more sense, thanks Pablo.
Nit below.

> +u16 nft_chain_offload_priority(struct nft_base_chain *basechain)
> +{
> +	u16 prio;
> +
> +	if (basechain->ops.priority < NFT_BASECHAIN_OFFLOAD_PRIO_MIN ||
> +	    basechain->ops.priority > NFT_BASECHAIN_OFFLOAD_PRIO_MAX)
> +		return 0;
> +
> +	/* map netfilter chain priority to hardware priority. */
> +	prio = basechain->ops.priority +
> +		NFT_BASECHAIN_OFFLOAD_PRIO_MAX +
> +			NFT_BASECHAIN_OFFLOAD_HW_PRIO_BASE;

Weird indent here.

> +
> +	return prio;
> +}

Re: [PATCH nf] netfilter: nf_tables: map basechain priority to hardware priority

[Jakub Kicinski]

On Tue, 30 Jul 2019 12:54:17 +0200, Pablo Neira Ayuso wrote:
> This patch maps basechain netfilter priorities from -8192 to 8191 to
> hardware priority 0xC000 + 1. tcf_auto_prio() uses 0xC000 if the user
> specifies no priority, then it subtract 1 for each new tcf_proto object.
> This patch uses the hardware priority range from 0xC000 to 0xFFFF for
> netfilter.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> This follows a rather conservative approach, I could just expose the
> 2^16 hardware priority range, but we may need to split this priority
> range among the ethtool_rx, tc and netfilter subsystems to start with
> and it should be possible to extend the priority range later on.
> 
> By netfilter priority, I'm refering to the basechain priority:
> 
> 	add chain x y { type filter hook ingress device eth0 priority 0; }
>                                                              ^^^^^^^^^^^
> 
> This is no transparently mapped to hardware, this patch shifts it to
> make it fit into the 0xC000 + 1 .. 0xFFFF hardware priority range.

Mmm.. so the ordering of tables is intended to be decided by priority
and not block type (nft, tc, ethtool)?  I was always expecting we 
would just follow the software order when it comes to inter-subsystem
decisions.  So ethtool first, then XDP, then TC, then nft, then
bridging etc. TC vs NFT based on:

static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
				    struct packet_type **ppt_prev)
{
...
	if (static_branch_unlikely(&ingress_needed_key)) {
		skb = sch_handle_ingress(skb, &pt_prev, &ret, orig_dev);
		if (!skb)
			goto out;

		if (nf_ingress(skb, &pt_prev, &ret, orig_dev) < 0)
			goto out;
	}

Are they solid use cases for choosing the ordering arbitrarily?