[nf-next PATCH v2 0/2] nf_tables: Export rule optimizer results to user space

[nf-next PATCH v2 0/2] nf_tables: Export rule optimizer results to user space

[Phil Sutter]

Changes since v1:
- Fixed two bugs in patch 2.

While transforming rules into binary blob, code checks if certain
expressions may be omitted. Any bugs in this code might lead to very
subtle breakage of firewall rulesets, so a way of asserting optimizer
correctness is highly necessary.

This series achieves this in the most minimal way by annotating omitted
expressions with a flag. Integrated into libnftnl print output,
testsuites in user space may verify optimizer effect and assert
correctness.

First patch introduces an expression flags attribute, second patch
implements the annotation itself.

Phil Sutter (2):
  netfilter: nf_tables: Introduce expression flags
  netfilter: nf_tables: Annotate reduced expressions

 include/net/netfilter/nf_tables.h        |  1 +
 include/uapi/linux/netfilter/nf_tables.h |  8 ++++++++
 net/netfilter/nf_tables_api.c            | 12 ++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.34.1

[nf-next PATCH v2 1/2] netfilter: nf_tables: Introduce expression flags

[Phil Sutter]

Allow dumping some info bits about expressions to user space.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/netfilter/nf_tables.h        | 1 +
 include/uapi/linux/netfilter/nf_tables.h | 1 +
 net/netfilter/nf_tables_api.c            | 4 ++++
 3 files changed, 6 insertions(+)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 20af9d3557b9d..78db54737de00 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -346,6 +346,7 @@ struct nft_set_estimate {
  */
 struct nft_expr {
 	const struct nft_expr_ops	*ops;
+	u32				flags;
 	unsigned char			data[]
 		__attribute__((aligned(__alignof__(u64))));
 };
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 466fd3f4447c2..36bf019322a44 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -518,6 +518,7 @@ enum nft_expr_attributes {
 	NFTA_EXPR_UNSPEC,
 	NFTA_EXPR_NAME,
 	NFTA_EXPR_DATA,
+	NFTA_EXPR_FLAGS,
 	__NFTA_EXPR_MAX
 };
 #define NFTA_EXPR_MAX		(__NFTA_EXPR_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f3ad02a399f8a..fddc557983119 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2731,6 +2731,7 @@ static const struct nft_expr_type *nft_expr_type_get(struct net *net,
 static const struct nla_policy nft_expr_policy[NFTA_EXPR_MAX + 1] = {
 	[NFTA_EXPR_NAME]	= { .type = NLA_STRING,
 				    .len = NFT_MODULE_AUTOLOAD_LIMIT },
+	[NFTA_EXPR_FLAGS]	= { .type = NLA_U32 },
 	[NFTA_EXPR_DATA]	= { .type = NLA_NESTED },
 };
 
@@ -2740,6 +2741,9 @@ static int nf_tables_fill_expr_info(struct sk_buff *skb,
 	if (nla_put_string(skb, NFTA_EXPR_NAME, expr->ops->type->name))
 		goto nla_put_failure;
 
+	if (nla_put_u32(skb, NFTA_EXPR_FLAGS, expr->flags))
+		goto nla_put_failure;
+
 	if (expr->ops->dump) {
 		struct nlattr *data = nla_nest_start_noflag(skb,
 							    NFTA_EXPR_DATA);
-- 
2.34.1

[nf-next PATCH v2 2/2] netfilter: nf_tables: Annotate reduced expressions

[Phil Sutter]

Introduce NFTA_EXPR_FLAG_REDUCED and set it for expressions which were
omitted from the rule blob due to being redundant. This allows user
space to verify the rule optimizer's results.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Keep pointers in struct nft_regs_track const and avoid assigning from
  track.cur to expr instead.
- Fix for situations where nft_expr_reduce() causes skipping of multiple
  expressions (payload + bitwise for instance).
---
 include/uapi/linux/netfilter/nf_tables.h | 7 +++++++
 net/netfilter/nf_tables_api.c            | 8 ++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 36bf019322a44..1da84ebc3f27a 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -523,6 +523,13 @@ enum nft_expr_attributes {
 };
 #define NFTA_EXPR_MAX		(__NFTA_EXPR_MAX - 1)
 
+/**
+ * NFTA_EXPR_FLAGS values
+ *
+ * @NFTA_EXPR_FLAG_REDUCED: redundant expression omitted from blob
+ */
+#define NFTA_EXPR_FLAG_REDUCED	(1 << 0)
+
 /**
  * enum nft_immediate_attributes - nf_tables immediate expression netlink attributes
  *
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index fddc557983119..d4fd32cf74d69 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8360,8 +8360,8 @@ static bool nft_expr_reduce(struct nft_regs_track *track,
 
 static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *chain)
 {
-	const struct nft_expr *expr, *last;
 	struct nft_regs_track track = {};
+	struct nft_expr *expr, *last;
 	unsigned int size, data_size;
 	void *data, *data_boundary;
 	struct nft_rule_dp *prule;
@@ -8404,7 +8404,11 @@ static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
 			track.cur = expr;
 
 			if (nft_expr_reduce(&track, expr)) {
-				expr = track.cur;
+				expr->flags |= NFTA_EXPR_FLAG_REDUCED;
+				while (expr != track.cur) {
+					expr = nft_expr_next(expr);
+					expr->flags |= NFTA_EXPR_FLAG_REDUCED;
+				}
 				continue;
 			}
 
-- 
2.34.1

Re: [nf-next PATCH v2 1/2] netfilter: nf_tables: Introduce expression flags

[Pablo Neira Ayuso]

On Thu, May 12, 2022 at 02:30:02PM +0200, Phil Sutter wrote:
> Allow dumping some info bits about expressions to user space.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/net/netfilter/nf_tables.h        | 1 +
>  include/uapi/linux/netfilter/nf_tables.h | 1 +
>  net/netfilter/nf_tables_api.c            | 4 ++++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 20af9d3557b9d..78db54737de00 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -346,6 +346,7 @@ struct nft_set_estimate {
>   */
>  struct nft_expr {
>  	const struct nft_expr_ops	*ops;
> +	u32				flags;

Could you add a new structure? Add struct nft_expr_dp and use it from
nft_rule_dp, so it is only the control plan representation that stores
this flag.

It will be a bit more work, but I think it is worth to keep the size
of the datapath representation as small as possible.

>  	unsigned char			data[]
>  		__attribute__((aligned(__alignof__(u64))));
>  };
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 466fd3f4447c2..36bf019322a44 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -518,6 +518,7 @@ enum nft_expr_attributes {
>  	NFTA_EXPR_UNSPEC,
>  	NFTA_EXPR_NAME,
>  	NFTA_EXPR_DATA,
> +	NFTA_EXPR_FLAGS,
>  	__NFTA_EXPR_MAX
>  };
>  #define NFTA_EXPR_MAX		(__NFTA_EXPR_MAX - 1)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index f3ad02a399f8a..fddc557983119 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2731,6 +2731,7 @@ static const struct nft_expr_type *nft_expr_type_get(struct net *net,
>  static const struct nla_policy nft_expr_policy[NFTA_EXPR_MAX + 1] = {
>  	[NFTA_EXPR_NAME]	= { .type = NLA_STRING,
>  				    .len = NFT_MODULE_AUTOLOAD_LIMIT },
> +	[NFTA_EXPR_FLAGS]	= { .type = NLA_U32 },
>  	[NFTA_EXPR_DATA]	= { .type = NLA_NESTED },
>  };
>  
> @@ -2740,6 +2741,9 @@ static int nf_tables_fill_expr_info(struct sk_buff *skb,
>  	if (nla_put_string(skb, NFTA_EXPR_NAME, expr->ops->type->name))
>  		goto nla_put_failure;
>  
> +	if (nla_put_u32(skb, NFTA_EXPR_FLAGS, expr->flags))
> +		goto nla_put_failure;
> +
>  	if (expr->ops->dump) {
>  		struct nlattr *data = nla_nest_start_noflag(skb,
>  							    NFTA_EXPR_DATA);
> -- 
> 2.34.1
> 

Re: [nf-next PATCH v2 1/2] netfilter: nf_tables: Introduce expression flags

[Phil Sutter]

On Thu, May 12, 2022 at 02:34:11PM +0200, Pablo Neira Ayuso wrote:
> On Thu, May 12, 2022 at 02:30:02PM +0200, Phil Sutter wrote:
> > Allow dumping some info bits about expressions to user space.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/net/netfilter/nf_tables.h        | 1 +
> >  include/uapi/linux/netfilter/nf_tables.h | 1 +
> >  net/netfilter/nf_tables_api.c            | 4 ++++
> >  3 files changed, 6 insertions(+)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index 20af9d3557b9d..78db54737de00 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -346,6 +346,7 @@ struct nft_set_estimate {
> >   */
> >  struct nft_expr {
> >  	const struct nft_expr_ops	*ops;
> > +	u32				flags;
> 
> Could you add a new structure? Add struct nft_expr_dp and use it from
> nft_rule_dp, so it is only the control plan representation that stores
> this flag.
> 
> It will be a bit more work, but I think it is worth to keep the size
> of the datapath representation as small as possible.

Sounds reasonable, but will get ugly: expr->ops->size includes struct
nft_expr size already, also real per-expr size is aligned to that
struct's size.

We could make expr->ops->size the real (unaligned) size value and change
size calculation in nf_tables_newrule() to add struct and alignment.
Then nf_tables_commit_chain_prepare() could iterate over the rule's
expressions and do its own size calculation for chain->blob_next size.

Do you see a better way to solve this?

Thanks, Phil