[nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

Allow for user space to provide an improved variant of the rule for
actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
compatibility for old user space tools (e.g. in outdated containers).

The new attribute is also dumped back to user space, e.g. for comparison
against the compatible variant.

While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Rename the new attribute as suggested
- Avoid the changing attribute meaning - ACTUAL_EXPR will always contain
  the new variant, both on input and output
- Rename the new struct nft_rule field accordingly
- Add the new attribute to nft_rule_policy
---
 include/net/netfilter/nf_tables.h        | 13 ++++++++
 include/uapi/linux/netfilter/nf_tables.h |  3 ++
 net/netfilter/nf_tables_api.c            | 38 ++++++++++++++++++++----
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index e69ce23566eab..e8446abb7620c 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -946,10 +946,22 @@ struct nft_expr_ops {
 	void				*data;
 };
 
+/**
+ *	struct nft_dump_expr - compat expression blob for rule dumps
+ *
+ *	@dlen: length of @data
+ *	@data: blob used as payload of NFTA_RULE_EXPRESSIONS attribute
+ */
+struct nft_dump_expr {
+	int	dlen;
+	char	data[];
+};
+
 /**
  *	struct nft_rule - nf_tables rule
  *
  *	@list: used internally
+ *	@dump_expr: Expression blob to dump instead of live data
  *	@handle: rule handle
  *	@genmask: generation mask
  *	@dlen: length of expression data
@@ -958,6 +970,7 @@ struct nft_expr_ops {
  */
 struct nft_rule {
 	struct list_head		list;
+	struct nft_dump_expr		*dump_expr;
 	u64				handle:42,
 					genmask:2,
 					dlen:12,
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index cfa844da1ce61..3e0fc9e8784cb 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -247,6 +247,8 @@ enum nft_chain_attributes {
  * @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
  * @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
  * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
+ * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
+ * @NFTA_RULE_ACTUAL_EXPR: list of expressions to really use if @NFTA_RULE_EXPRESSIONS must contain a compatible representation of the rule (NLA_NESTED: nft_expr_attributes)
  */
 enum nft_rule_attributes {
 	NFTA_RULE_UNSPEC,
@@ -261,6 +263,7 @@ enum nft_rule_attributes {
 	NFTA_RULE_ID,
 	NFTA_RULE_POSITION_ID,
 	NFTA_RULE_CHAIN_ID,
+	NFTA_RULE_ACTUAL_EXPR,
 	__NFTA_RULE_MAX
 };
 #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6269b0d9977c6..b3a14819f91f8 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3019,7 +3019,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
 				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
 	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
-	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
+	[NFTA_RULE_EXPRESSIONS]	= NLA_POLICY_NESTED_ARRAY(nft_expr_policy),
 	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
 	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
 	[NFTA_RULE_USERDATA]	= { .type = NLA_BINARY,
@@ -3027,6 +3027,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
 	[NFTA_RULE_ID]		= { .type = NLA_U32 },
 	[NFTA_RULE_POSITION_ID]	= { .type = NLA_U32 },
 	[NFTA_RULE_CHAIN_ID]	= { .type = NLA_U32 },
+	[NFTA_RULE_ACTUAL_EXPR]	= NLA_POLICY_NESTED_ARRAY(nft_expr_policy),
 };
 
 static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
@@ -3064,9 +3065,18 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
 	if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
 		nft_flow_rule_stats(chain, rule);
 
-	list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
-	if (list == NULL)
+	if (rule->dump_expr) {
+		if (nla_put(skb, NFTA_RULE_EXPRESSIONS,
+			    rule->dump_expr->dlen, rule->dump_expr->data) < 0)
+			goto nla_put_failure;
+
+		list = nla_nest_start_noflag(skb, NFTA_RULE_ACTUAL_EXPR);
+	} else {
+		list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
+	}
+	if (!list)
 		goto nla_put_failure;
+
 	nft_rule_for_each_expr(expr, next, rule) {
 		if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
 			goto nla_put_failure;
@@ -3366,6 +3376,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
 		nf_tables_expr_destroy(ctx, expr);
 		expr = next;
 	}
+	kfree(rule->dump_expr);
 	kfree(rule);
 }
 
@@ -3443,7 +3454,9 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 	struct nft_rule *rule, *old_rule = NULL;
 	struct nft_expr_info *expr_info = NULL;
 	u8 family = info->nfmsg->nfgen_family;
+	struct nft_dump_expr *dump_expr = NULL;
 	struct nft_flow_rule *flow = NULL;
+	const struct nlattr *expr_nla;
 	struct net *net = info->net;
 	struct nft_userdata *udata;
 	struct nft_table *table;
@@ -3529,14 +3542,15 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 
 	n = 0;
 	size = 0;
-	if (nla[NFTA_RULE_EXPRESSIONS]) {
+	expr_nla = nla[NFTA_RULE_ACTUAL_EXPR] ?: nla[NFTA_RULE_EXPRESSIONS];
+	if (expr_nla) {
 		expr_info = kvmalloc_array(NFT_RULE_MAXEXPRS,
 					   sizeof(struct nft_expr_info),
 					   GFP_KERNEL);
 		if (!expr_info)
 			return -ENOMEM;
 
-		nla_for_each_nested(tmp, nla[NFTA_RULE_EXPRESSIONS], rem) {
+		nla_for_each_nested(tmp, expr_nla, rem) {
 			err = -EINVAL;
 			if (nla_type(tmp) != NFTA_LIST_ELEM)
 				goto err_release_expr;
@@ -3556,6 +3570,19 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 	if (size >= 1 << 12)
 		goto err_release_expr;
 
+	if (nla[NFTA_RULE_ACTUAL_EXPR]) {
+		int dlen = nla_len(nla[NFTA_RULE_EXPRESSIONS]);
+
+		/* store unused NFTA_RULE_EXPRESSIONS for later */
+		dump_expr = kvmalloc(sizeof(*dump_expr) + dlen, GFP_KERNEL);
+		if (!dump_expr) {
+			err = -ENOMEM;
+			goto err_release_expr;
+		}
+		dump_expr->dlen = dlen;
+		nla_memcpy(dump_expr->data, nla[NFTA_RULE_EXPRESSIONS], dlen);
+	}
+
 	if (nla[NFTA_RULE_USERDATA]) {
 		ulen = nla_len(nla[NFTA_RULE_USERDATA]);
 		if (ulen > 0)
@@ -3572,6 +3599,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
 	rule->handle = handle;
 	rule->dlen   = size;
 	rule->udata  = ulen ? 1 : 0;
+	rule->dump_expr = dump_expr;
 
 	if (ulen) {
 		udata = nft_userdata(rule);
-- 
2.38.0

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

Bump?

On Wed, Dec 21, 2022 at 03:22:21PM +0100, Phil Sutter wrote:
> Allow for user space to provide an improved variant of the rule for
> actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
> compatibility for old user space tools (e.g. in outdated containers).
> 
> The new attribute is also dumped back to user space, e.g. for comparison
> against the compatible variant.
> 
> While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Rename the new attribute as suggested
> - Avoid the changing attribute meaning - ACTUAL_EXPR will always contain
>   the new variant, both on input and output
> - Rename the new struct nft_rule field accordingly
> - Add the new attribute to nft_rule_policy
> ---
>  include/net/netfilter/nf_tables.h        | 13 ++++++++
>  include/uapi/linux/netfilter/nf_tables.h |  3 ++
>  net/netfilter/nf_tables_api.c            | 38 ++++++++++++++++++++----
>  3 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index e69ce23566eab..e8446abb7620c 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -946,10 +946,22 @@ struct nft_expr_ops {
>  	void				*data;
>  };
>  
> +/**
> + *	struct nft_dump_expr - compat expression blob for rule dumps
> + *
> + *	@dlen: length of @data
> + *	@data: blob used as payload of NFTA_RULE_EXPRESSIONS attribute
> + */
> +struct nft_dump_expr {
> +	int	dlen;
> +	char	data[];
> +};
> +
>  /**
>   *	struct nft_rule - nf_tables rule
>   *
>   *	@list: used internally
> + *	@dump_expr: Expression blob to dump instead of live data
>   *	@handle: rule handle
>   *	@genmask: generation mask
>   *	@dlen: length of expression data
> @@ -958,6 +970,7 @@ struct nft_expr_ops {
>   */
>  struct nft_rule {
>  	struct list_head		list;
> +	struct nft_dump_expr		*dump_expr;
>  	u64				handle:42,
>  					genmask:2,
>  					dlen:12,
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index cfa844da1ce61..3e0fc9e8784cb 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -247,6 +247,8 @@ enum nft_chain_attributes {
>   * @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
>   * @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
>   * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule (NLA_U32)
> + * @NFTA_RULE_CHAIN_ID: add the rule to chain by ID, alternative to @NFTA_RULE_CHAIN (NLA_U32)
> + * @NFTA_RULE_ACTUAL_EXPR: list of expressions to really use if @NFTA_RULE_EXPRESSIONS must contain a compatible representation of the rule (NLA_NESTED: nft_expr_attributes)
>   */
>  enum nft_rule_attributes {
>  	NFTA_RULE_UNSPEC,
> @@ -261,6 +263,7 @@ enum nft_rule_attributes {
>  	NFTA_RULE_ID,
>  	NFTA_RULE_POSITION_ID,
>  	NFTA_RULE_CHAIN_ID,
> +	NFTA_RULE_ACTUAL_EXPR,
>  	__NFTA_RULE_MAX
>  };
>  #define NFTA_RULE_MAX		(__NFTA_RULE_MAX - 1)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 6269b0d9977c6..b3a14819f91f8 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -3019,7 +3019,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
>  	[NFTA_RULE_CHAIN]	= { .type = NLA_STRING,
>  				    .len = NFT_CHAIN_MAXNAMELEN - 1 },
>  	[NFTA_RULE_HANDLE]	= { .type = NLA_U64 },
> -	[NFTA_RULE_EXPRESSIONS]	= { .type = NLA_NESTED },
> +	[NFTA_RULE_EXPRESSIONS]	= NLA_POLICY_NESTED_ARRAY(nft_expr_policy),
>  	[NFTA_RULE_COMPAT]	= { .type = NLA_NESTED },
>  	[NFTA_RULE_POSITION]	= { .type = NLA_U64 },
>  	[NFTA_RULE_USERDATA]	= { .type = NLA_BINARY,
> @@ -3027,6 +3027,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
>  	[NFTA_RULE_ID]		= { .type = NLA_U32 },
>  	[NFTA_RULE_POSITION_ID]	= { .type = NLA_U32 },
>  	[NFTA_RULE_CHAIN_ID]	= { .type = NLA_U32 },
> +	[NFTA_RULE_ACTUAL_EXPR]	= NLA_POLICY_NESTED_ARRAY(nft_expr_policy),
>  };
>  
>  static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
> @@ -3064,9 +3065,18 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
>  	if (chain->flags & NFT_CHAIN_HW_OFFLOAD)
>  		nft_flow_rule_stats(chain, rule);
>  
> -	list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
> -	if (list == NULL)
> +	if (rule->dump_expr) {
> +		if (nla_put(skb, NFTA_RULE_EXPRESSIONS,
> +			    rule->dump_expr->dlen, rule->dump_expr->data) < 0)
> +			goto nla_put_failure;
> +
> +		list = nla_nest_start_noflag(skb, NFTA_RULE_ACTUAL_EXPR);
> +	} else {
> +		list = nla_nest_start_noflag(skb, NFTA_RULE_EXPRESSIONS);
> +	}
> +	if (!list)
>  		goto nla_put_failure;
> +
>  	nft_rule_for_each_expr(expr, next, rule) {
>  		if (nft_expr_dump(skb, NFTA_LIST_ELEM, expr, reset) < 0)
>  			goto nla_put_failure;
> @@ -3366,6 +3376,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
>  		nf_tables_expr_destroy(ctx, expr);
>  		expr = next;
>  	}
> +	kfree(rule->dump_expr);
>  	kfree(rule);
>  }
>  
> @@ -3443,7 +3454,9 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
>  	struct nft_rule *rule, *old_rule = NULL;
>  	struct nft_expr_info *expr_info = NULL;
>  	u8 family = info->nfmsg->nfgen_family;
> +	struct nft_dump_expr *dump_expr = NULL;
>  	struct nft_flow_rule *flow = NULL;
> +	const struct nlattr *expr_nla;
>  	struct net *net = info->net;
>  	struct nft_userdata *udata;
>  	struct nft_table *table;
> @@ -3529,14 +3542,15 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
>  
>  	n = 0;
>  	size = 0;
> -	if (nla[NFTA_RULE_EXPRESSIONS]) {
> +	expr_nla = nla[NFTA_RULE_ACTUAL_EXPR] ?: nla[NFTA_RULE_EXPRESSIONS];
> +	if (expr_nla) {
>  		expr_info = kvmalloc_array(NFT_RULE_MAXEXPRS,
>  					   sizeof(struct nft_expr_info),
>  					   GFP_KERNEL);
>  		if (!expr_info)
>  			return -ENOMEM;
>  
> -		nla_for_each_nested(tmp, nla[NFTA_RULE_EXPRESSIONS], rem) {
> +		nla_for_each_nested(tmp, expr_nla, rem) {
>  			err = -EINVAL;
>  			if (nla_type(tmp) != NFTA_LIST_ELEM)
>  				goto err_release_expr;
> @@ -3556,6 +3570,19 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
>  	if (size >= 1 << 12)
>  		goto err_release_expr;
>  
> +	if (nla[NFTA_RULE_ACTUAL_EXPR]) {
> +		int dlen = nla_len(nla[NFTA_RULE_EXPRESSIONS]);
> +
> +		/* store unused NFTA_RULE_EXPRESSIONS for later */
> +		dump_expr = kvmalloc(sizeof(*dump_expr) + dlen, GFP_KERNEL);
> +		if (!dump_expr) {
> +			err = -ENOMEM;
> +			goto err_release_expr;
> +		}
> +		dump_expr->dlen = dlen;
> +		nla_memcpy(dump_expr->data, nla[NFTA_RULE_EXPRESSIONS], dlen);
> +	}
> +
>  	if (nla[NFTA_RULE_USERDATA]) {
>  		ulen = nla_len(nla[NFTA_RULE_USERDATA]);
>  		if (ulen > 0)
> @@ -3572,6 +3599,7 @@ static int nf_tables_newrule(struct sk_buff *skb, const struct nfnl_info *info,
>  	rule->handle = handle;
>  	rule->dlen   = size;
>  	rule->udata  = ulen ? 1 : 0;
> +	rule->dump_expr = dump_expr;
>  
>  	if (ulen) {
>  		udata = nft_userdata(rule);
> -- 
> 2.38.0
> 
> 

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Thu, Jan 12, 2023 at 11:15:10AM +0100, Phil Sutter wrote:
> Bump?
> 
> On Wed, Dec 21, 2022 at 03:22:21PM +0100, Phil Sutter wrote:
> > Allow for user space to provide an improved variant of the rule for
> > actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
> > compatibility for old user space tools (e.g. in outdated containers).
> > 
> > The new attribute is also dumped back to user space, e.g. for comparison
> > against the compatible variant.
> > 
> > While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.

Could you split this in two patches?

I still don't see how this is improving the situation for the scenario
you describe, if you could extend a bit on how you plan to use this
I'd appreciate.

Thanks.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Thu, Jan 12, 2023 at 12:06:55PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 12, 2023 at 11:15:10AM +0100, Phil Sutter wrote:
> > Bump?
> > 
> > On Wed, Dec 21, 2022 at 03:22:21PM +0100, Phil Sutter wrote:
> > > Allow for user space to provide an improved variant of the rule for
> > > actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
> > > compatibility for old user space tools (e.g. in outdated containers).
> > > 
> > > The new attribute is also dumped back to user space, e.g. for comparison
> > > against the compatible variant.
> > > 
> > > While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.
> 
> Could you split this in two patches?

Separate the nft_rule_policy_change? Sure!

> I still don't see how this is improving the situation for the scenario
> you describe, if you could extend a bit on how you plan to use this
> I'd appreciate.

I can send you my WiP libnftnl and iptables patches if that helps.

The approach this patch follows is pretty simple, though: The kernel
will accept NFTA_RULE_ACTUAL_EXPR to override NFTA_RULE_EXPRESSIONS for
use in the live ruleset.  When fetching the ruleset, old user space will
ignore NFTA_RULE_ACTUAL_EXPR, so new user space may submit a compatible
variant of the rule in NFTA_RULE_EXPRESSIONS and a modern variant in
NFTA_RULE_ACTUAL_EXPR.

In iptables, when converting a rule from iptables_command_state into
nftnl expressions, I insert all expressions into both
NFTA_RULE_EXPRESSIONS and NFTA_RULE_ACTUAL_EXPR unless an extension does
fancy stuff (e.g. was converted into native expressions).

My test piece is limit match which had to be converted once (see commit
5de8dcf75941c for details): I add the native expressions to
NFTA_RULE_ACTUAL_EXPR and create a compat "match" expression for
NFTA_RULE_EXPRESSIONS only.

The kernel will use the native expressions in the ruleset, dumps will
contain the compat "match" expression instead.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Thu, Jan 12, 2023 at 01:02:59PM +0100, Phil Sutter wrote:
> On Thu, Jan 12, 2023 at 12:06:55PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 12, 2023 at 11:15:10AM +0100, Phil Sutter wrote:
> > > Bump?
> > > 
> > > On Wed, Dec 21, 2022 at 03:22:21PM +0100, Phil Sutter wrote:
> > > > Allow for user space to provide an improved variant of the rule for
> > > > actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
> > > > compatibility for old user space tools (e.g. in outdated containers).
> > > > 
> > > > The new attribute is also dumped back to user space, e.g. for comparison
> > > > against the compatible variant.
> > > > 
> > > > While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.
> > 
> > Could you split this in two patches?
> 
> Separate the nft_rule_policy_change? Sure!

Thanks.

> > I still don't see how this is improving the situation for the scenario
> > you describe, if you could extend a bit on how you plan to use this
> > I'd appreciate.
> 
> I can send you my WiP libnftnl and iptables patches if that helps.
> 
> The approach this patch follows is pretty simple, though: The kernel
> will accept NFTA_RULE_ACTUAL_EXPR to override NFTA_RULE_EXPRESSIONS for
> use in the live ruleset.  When fetching the ruleset, old user space will
> ignore NFTA_RULE_ACTUAL_EXPR, so new user space may submit a compatible
> variant of the rule in NFTA_RULE_EXPRESSIONS and a modern variant in
> NFTA_RULE_ACTUAL_EXPR.

so _ACTUAL_EXPR is the modern representation, and _RULE_EXPRESSIONS
the old one?

Maybe the opposite is better? I mean, no changes in the
NFTA_RULE_EXPRESSIONS semantics, these are always the expressions that
run in the datapath, and the alternative expression representation is
just for backward compatibility?

Maybe all this can be handled from _USERDATA? I mean, to add the
netlink representation there?

> In iptables, when converting a rule from iptables_command_state into
> nftnl expressions, I insert all expressions into both
> NFTA_RULE_EXPRESSIONS and NFTA_RULE_ACTUAL_EXPR unless an extension does
> fancy stuff (e.g. was converted into native expressions).

So NFTA_RULE_EXPRESSIONS contains xt compat expression or is it
ACTUAL_EXPR?

Probably you can just add NFTA_RULE_COMPAT_EXPRS? This new attribute
provides a pure xt compat representation? _ACTUAL concept gets me
confused.

> My test piece is limit match which had to be converted once (see commit
> 5de8dcf75941c for details): I add the native expressions to
> NFTA_RULE_ACTUAL_EXPR and create a compat "match" expression for
> NFTA_RULE_EXPRESSIONS only.

What gets me confused is what the kernel actually uses from the
datapath.

> The kernel will use the native expressions in the ruleset, dumps will
> contain the compat "match" expression instead.

Both representations should be dumped, right? In my mind, userspace
just falls back to my proposed NFTA_RULE_COMPAT_EXPRS in case it
cannot decode NFTA_RULE_EXPRESSIONS.

Sorry for taking a while to come back here.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Wed, Jan 18, 2023 at 12:58:47PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 12, 2023 at 01:02:59PM +0100, Phil Sutter wrote:
> > On Thu, Jan 12, 2023 at 12:06:55PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Jan 12, 2023 at 11:15:10AM +0100, Phil Sutter wrote:
> > > > Bump?
> > > > 
> > > > On Wed, Dec 21, 2022 at 03:22:21PM +0100, Phil Sutter wrote:
> > > > > Allow for user space to provide an improved variant of the rule for
> > > > > actual use. The variant in NFTA_RULE_EXPRESSIONS may provide maximum
> > > > > compatibility for old user space tools (e.g. in outdated containers).
> > > > > 
> > > > > The new attribute is also dumped back to user space, e.g. for comparison
> > > > > against the compatible variant.
> > > > > 
> > > > > While being at it, improve nft_rule_policy for NFTA_RULE_EXPRESSIONS.
> > > 
> > > Could you split this in two patches?
> > 
> > Separate the nft_rule_policy_change? Sure!
> 
> Thanks.
> 
> > > I still don't see how this is improving the situation for the scenario
> > > you describe, if you could extend a bit on how you plan to use this
> > > I'd appreciate.
> > 
> > I can send you my WiP libnftnl and iptables patches if that helps.
> > 
> > The approach this patch follows is pretty simple, though: The kernel
> > will accept NFTA_RULE_ACTUAL_EXPR to override NFTA_RULE_EXPRESSIONS for
> > use in the live ruleset.  When fetching the ruleset, old user space will
> > ignore NFTA_RULE_ACTUAL_EXPR, so new user space may submit a compatible
> > variant of the rule in NFTA_RULE_EXPRESSIONS and a modern variant in
> > NFTA_RULE_ACTUAL_EXPR.
> 
> so _ACTUAL_EXPR is the modern representation, and _RULE_EXPRESSIONS
> the old one?
> 
> Maybe the opposite is better? I mean, no changes in the
> NFTA_RULE_EXPRESSIONS semantics, these are always the expressions that
> run in the datapath, and the alternative expression representation is
> just for backward compatibility?
> 
> Maybe all this can be handled from _USERDATA? I mean, to add the
> netlink representation there?

The crucial aspect of this implementation is to provide a compatible
rule representation for old software which is not aware of it. This is
only possible by dumping the compat representation in the well-known
NFTA_RULE_EXPRESSIONS attribute.

This means what is contained in NFTA_RULE_EXPRESSIONS may not be what
the kernel actually executes. To make this less scary, the kernel should
dump the actual rule in a second attribute for the sake of verification
in user space.

While rule dumps are pretty much fixed given the above, there is
flexibility when it comes to loading the rule:

A) Submit the compat representation as additional attribute

This was my initial approach, but Florian objected because the changing
content of NFTA_RULE_EXPRESSIONS attribute may be confusing:

On input, NFTA_RULE_EXPRESSIONS contains the new rule representation, on
output it contains the compat one. The extra attribute I introduced
behaves identical, i.e. on input it holds the compat representation
while on output it holds the new one.

B) Submit the new representation as additional attribute

This is the current approach: If the additional attribute is present,
the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
alone (actually: store it for dumps). Otherwise it will "fall back" to
using NFTA_RULE_EXPRESSIONS just as usual.

When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
will dump that as-is and serialize the active rule into an additional
attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
just as usual.

> > In iptables, when converting a rule from iptables_command_state into
> > nftnl expressions, I insert all expressions into both
> > NFTA_RULE_EXPRESSIONS and NFTA_RULE_ACTUAL_EXPR unless an extension does
> > fancy stuff (e.g. was converted into native expressions).
> 
> So NFTA_RULE_EXPRESSIONS contains xt compat expression or is it
> ACTUAL_EXPR?
> 
> Probably you can just add NFTA_RULE_COMPAT_EXPRS? This new attribute
> provides a pure xt compat representation? _ACTUAL concept gets me
> confused.

See above. I hope it clarifies things.

> > My test piece is limit match which had to be converted once (see commit
> > 5de8dcf75941c for details): I add the native expressions to
> > NFTA_RULE_ACTUAL_EXPR and create a compat "match" expression for
> > NFTA_RULE_EXPRESSIONS only.
> 
> What gets me confused is what the kernel actually uses from the
> datapath.
> 
> > The kernel will use the native expressions in the ruleset, dumps will
> > contain the compat "match" expression instead.
> 
> Both representations should be dumped, right? In my mind, userspace
> just falls back to my proposed NFTA_RULE_COMPAT_EXPRS in case it
> cannot decode NFTA_RULE_EXPRESSIONS.

I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
if present and puts NFTA_RULE_EXPRESSIONS into a second list for
verification only. In iptables, I parse both lists separately into
iptables_command_state objects and compare them. If not identical,
there's a bug.

> Sorry for taking a while to come back here.

No problem.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

Hi Phil,

On Wed, Jan 18, 2023 at 02:48:20PM +0100, Phil Sutter wrote:
[...]
> The crucial aspect of this implementation is to provide a compatible
> rule representation for old software which is not aware of it. This is
> only possible by dumping the compat representation in the well-known
> NFTA_RULE_EXPRESSIONS attribute.

OK, so NFTA_RULE_EXPRESSIONS contains the xt expressions.

Then, _ACTUAL_EXPR is taken if kernel supports it and these are
expressions that run from datapath, if present.

> This means what is contained in NFTA_RULE_EXPRESSIONS may not be what
> the kernel actually executes. To make this less scary, the kernel should
> dump the actual rule in a second attribute for the sake of verification
> in user space.
>
> While rule dumps are pretty much fixed given the above, there is
> flexibility when it comes to loading the rule:
> 
> A) Submit the compat representation as additional attribute
> 
> This was my initial approach, but Florian objected because the changing
> content of NFTA_RULE_EXPRESSIONS attribute may be confusing:

It is indeed.

> On input, NFTA_RULE_EXPRESSIONS contains the new rule representation, on
> output it contains the compat one. The extra attribute I introduced
> behaves identical, i.e. on input it holds the compat representation
> while on output it holds the new one.
> 
> B) Submit the new representation as additional attribute
> 
> This is the current approach: If the additional attribute is present,
> the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
> alone (actually: store it for dumps). Otherwise it will "fall back" to
> using NFTA_RULE_EXPRESSIONS just as usual.
>
> When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
> will dump that as-is and serialize the active rule into an additional
> attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
> just as usual.

So this is not swapping things, right? Probably I am still getting
confused but the initial approach described in A.

When, dumping back to userspace, NFTA_RULE_EXPRESSIONS still stores
the xt compat representation and NFTA_RULE_ACTUAL_EXPRS the one that
runs from kernel datapath (if the kernel supports this attribute).

[...]
> I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
> if present and puts NFTA_RULE_EXPRESSIONS into a second list for
> verification only. In iptables, I parse both lists separately into
> iptables_command_state objects and compare them. If not identical,
> there's a bug.

Old kernels would simply discard the ACTUAL_ attribute. Maybe _ALT_
standing by alternative is a better name?

Sorry, this is a bit confusing but I understand something like this is
required as you explained during the NFWS.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

Hi Pablo,

On Thu, Feb 02, 2023 at 10:31:58PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 18, 2023 at 02:48:20PM +0100, Phil Sutter wrote:
> [...]
> > The crucial aspect of this implementation is to provide a compatible
> > rule representation for old software which is not aware of it. This is
> > only possible by dumping the compat representation in the well-known
> > NFTA_RULE_EXPRESSIONS attribute.
> 
> OK, so NFTA_RULE_EXPRESSIONS contains the xt expressions.
> 
> Then, _ACTUAL_EXPR is taken if kernel supports it and these are
> expressions that run from datapath, if present.

Yes, this is indeed somewhat of a downside of this approach: a kernel
which doesn't support the new attribute will use the compatible version
of the rule instead of the improved one. But apart from that, everything
just works.

> > This means what is contained in NFTA_RULE_EXPRESSIONS may not be what
> > the kernel actually executes. To make this less scary, the kernel should
> > dump the actual rule in a second attribute for the sake of verification
> > in user space.
> >
> > While rule dumps are pretty much fixed given the above, there is
> > flexibility when it comes to loading the rule:
> > 
> > A) Submit the compat representation as additional attribute
> > 
> > This was my initial approach, but Florian objected because the changing
> > content of NFTA_RULE_EXPRESSIONS attribute may be confusing:
> 
> It is indeed.

Sorry. :(

> > On input, NFTA_RULE_EXPRESSIONS contains the new rule representation, on
> > output it contains the compat one. The extra attribute I introduced
> > behaves identical, i.e. on input it holds the compat representation
> > while on output it holds the new one.
> > 
> > B) Submit the new representation as additional attribute
> > 
> > This is the current approach: If the additional attribute is present,
> > the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
> > alone (actually: store it for dumps). Otherwise it will "fall back" to
> > using NFTA_RULE_EXPRESSIONS just as usual.
> >
> > When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
> > will dump that as-is and serialize the active rule into an additional
> > attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
> > just as usual.
> 
> So this is not swapping things, right? Probably I am still getting
> confused but the initial approach described in A.

No swap: The kernel will dump in NFTA_RULE_EXPRESSIONS exactly what it
got in that attribute, same for the new one.

> When, dumping back to userspace, NFTA_RULE_EXPRESSIONS still stores
> the xt compat representation and NFTA_RULE_ACTUAL_EXPRS the one that
> runs from kernel datapath (if the kernel supports this attribute).

Yes, exactly. And old user space or nft will put the "new"
representation into NFTA_RULE_EXPRESSIONS, not attach
NFTA_RULE_ACTUAL_EXPRS and thus the kernel will use the former in its
data path.

> [...]
> > I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
> > if present and puts NFTA_RULE_EXPRESSIONS into a second list for
> > verification only. In iptables, I parse both lists separately into
> > iptables_command_state objects and compare them. If not identical,
> > there's a bug.
> 
> Old kernels would simply discard the ACTUAL_ attribute. Maybe _ALT_
> standing by alternative is a better name?

Fine with me! "ACTUAL" was suggested by Florian, probably to point out
that it's what should take precedence if present. In my understanding,
"ALT" means "as good as".

> Sorry, this is a bit confusing but I understand something like this is
> required as you explained during the NFWS.

Thanks. Irrespective of the "crazy container people" mixing iptables
versions and variants like mad, I believe it will allow us to make more
drastic changes in future.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Fri, Feb 03, 2023 at 02:48:30PM +0100, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Feb 02, 2023 at 10:31:58PM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Jan 18, 2023 at 02:48:20PM +0100, Phil Sutter wrote:
> > [...]
> > > The crucial aspect of this implementation is to provide a compatible
> > > rule representation for old software which is not aware of it. This is
> > > only possible by dumping the compat representation in the well-known
> > > NFTA_RULE_EXPRESSIONS attribute.
> > 
> > OK, so NFTA_RULE_EXPRESSIONS contains the xt expressions.
> > 
> > Then, _ACTUAL_EXPR is taken if kernel supports it and these are
> > expressions that run from datapath, if present.
> 
> Yes, this is indeed somewhat of a downside of this approach: a kernel
> which doesn't support the new attribute will use the compatible version
> of the rule instead of the improved one. But apart from that, everything
> just works.

For old kernels, this behaviour is expected.

[...]
> > > B) Submit the new representation as additional attribute
> > > 
> > > This is the current approach: If the additional attribute is present,
> > > the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
> > > alone (actually: store it for dumps). Otherwise it will "fall back" to
> > > using NFTA_RULE_EXPRESSIONS just as usual.
> > >
> > > When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
> > > will dump that as-is and serialize the active rule into an additional
> > > attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
> > > just as usual.
> > 
> > So this is not swapping things, right? Probably I am still getting
> > confused but the initial approach described in A.
> 
> No swap: The kernel will dump in NFTA_RULE_EXPRESSIONS exactly what it
> got in that attribute, same for the new one.

Good.

> > When, dumping back to userspace, NFTA_RULE_EXPRESSIONS still stores
> > the xt compat representation and NFTA_RULE_ACTUAL_EXPRS the one that
> > runs from kernel datapath (if the kernel supports this attribute).
> 
> Yes, exactly. And old user space or nft will put the "new"
> representation into NFTA_RULE_EXPRESSIONS, not attach
> NFTA_RULE_ACTUAL_EXPRS and thus the kernel will use the former in its
> data path.

That is:

= New kernels / new userspace =

- NFTA_RULE_EXPRESSIONS is used if no NFTA_RULE_ACTUAL_EXPRS is provided.
- if NFTA_RULE_ACTUAL_EXPRS is provided, then it is used.

= New kernels / old userspace =

- NFTA_RULE_EXPRESSIONS is always used.

= Old kernels / new userspace =

- NFTA_RULE_EXPRESSIONS is always used, NFTA_RULE_ACTUAL_EXPRS is ignored.

> > [...]
> > > I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
> > > if present and puts NFTA_RULE_EXPRESSIONS into a second list for
> > > verification only. In iptables, I parse both lists separately into
> > > iptables_command_state objects and compare them. If not identical,
> > > there's a bug.
> > 
> > Old kernels would simply discard the ACTUAL_ attribute. Maybe _ALT_
> > standing by alternative is a better name?
> 
> Fine with me! "ACTUAL" was suggested by Florian, probably to point out
> that it's what should take precedence if present. In my understanding,
> "ALT" means "as good as".

From old kernel perspective, this is an alternative representation (it
can be ignored).  From new kernel perspective, it is actual.

> > Sorry, this is a bit confusing but I understand something like this is
> > required as you explained during the NFWS.
> 
> Thanks. Irrespective of the "crazy container people" mixing iptables
> versions and variants like mad, I believe it will allow us to make more
> drastic changes in future.

Thanks for explaining. This will be also more work from userspace to
make sure both are consistent.

A user could abuse this API to add something completely different to
NFTA_RULE_EXPRESSIONS while providing NFTA_RULE_ACTUAL_EXPRS.

I also wonder if this might cause problems with nftables and implicit
sets, they are bound to one single lookup expression that, when gone,
the set is released. Now you will have two expressions pointing to an
implicit set. Same thing with implicit chains. This might get tricky
with the transaction interface.

iptables is rather simple representation (no sets), but nftables is
more expressive.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 03, 2023 at 02:48:30PM +0100, Phil Sutter wrote:
> > On Thu, Feb 02, 2023 at 10:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > On Wed, Jan 18, 2023 at 02:48:20PM +0100, Phil Sutter wrote:
> > > [...]
> > > > The crucial aspect of this implementation is to provide a compatible
> > > > rule representation for old software which is not aware of it. This is
> > > > only possible by dumping the compat representation in the well-known
> > > > NFTA_RULE_EXPRESSIONS attribute.
> > > 
> > > OK, so NFTA_RULE_EXPRESSIONS contains the xt expressions.
> > > 
> > > Then, _ACTUAL_EXPR is taken if kernel supports it and these are
> > > expressions that run from datapath, if present.
> > 
> > Yes, this is indeed somewhat of a downside of this approach: a kernel
> > which doesn't support the new attribute will use the compatible version
> > of the rule instead of the improved one. But apart from that, everything
> > just works.
> 
> For old kernels, this behaviour is expected.
> 
> [...]
> > > > B) Submit the new representation as additional attribute
> > > > 
> > > > This is the current approach: If the additional attribute is present,
> > > > the kernel will use it to build the rule and leave NFTA_RULE_EXPRESSIONS
> > > > alone (actually: store it for dumps). Otherwise it will "fall back" to
> > > > using NFTA_RULE_EXPRESSIONS just as usual.
> > > >
> > > > When dumping, if a stored NFTA_RULE_EXPRESSIONS content is present, it
> > > > will dump that as-is and serialize the active rule into an additional
> > > > attribute. Otherwise the active rule will go into NFTA_RULE_EXPRESSIONS
> > > > just as usual.
> > > 
> > > So this is not swapping things, right? Probably I am still getting
> > > confused but the initial approach described in A.
> > 
> > No swap: The kernel will dump in NFTA_RULE_EXPRESSIONS exactly what it
> > got in that attribute, same for the new one.
> 
> Good.
> 
> > > When, dumping back to userspace, NFTA_RULE_EXPRESSIONS still stores
> > > the xt compat representation and NFTA_RULE_ACTUAL_EXPRS the one that
> > > runs from kernel datapath (if the kernel supports this attribute).
> > 
> > Yes, exactly. And old user space or nft will put the "new"
> > representation into NFTA_RULE_EXPRESSIONS, not attach
> > NFTA_RULE_ACTUAL_EXPRS and thus the kernel will use the former in its
> > data path.
> 
> That is:
> 
> = New kernels / new userspace =
> 
> - NFTA_RULE_EXPRESSIONS is used if no NFTA_RULE_ACTUAL_EXPRS is provided.
> - if NFTA_RULE_ACTUAL_EXPRS is provided, then it is used.
> 
> = New kernels / old userspace =
> 
> - NFTA_RULE_EXPRESSIONS is always used.
> 
> = Old kernels / new userspace =
> 
> - NFTA_RULE_EXPRESSIONS is always used, NFTA_RULE_ACTUAL_EXPRS is ignored.

Correct.

> > > [...]
> > > > I am swapping things around in libnftnl - it uses NFTA_RULE_ACTUAL_EXPRS
> > > > if present and puts NFTA_RULE_EXPRESSIONS into a second list for
> > > > verification only. In iptables, I parse both lists separately into
> > > > iptables_command_state objects and compare them. If not identical,
> > > > there's a bug.
> > > 
> > > Old kernels would simply discard the ACTUAL_ attribute. Maybe _ALT_
> > > standing by alternative is a better name?
> > 
> > Fine with me! "ACTUAL" was suggested by Florian, probably to point out
> > that it's what should take precedence if present. In my understanding,
> > "ALT" means "as good as".
> 
> From old kernel perspective, this is an alternative representation (it
> can be ignored).  From new kernel perspective, it is actual.
> 
> > > Sorry, this is a bit confusing but I understand something like this is
> > > required as you explained during the NFWS.
> > 
> > Thanks. Irrespective of the "crazy container people" mixing iptables
> > versions and variants like mad, I believe it will allow us to make more
> > drastic changes in future.
> 
> Thanks for explaining. This will be also more work from userspace to
> make sure both are consistent.
> 
> A user could abuse this API to add something completely different to
> NFTA_RULE_EXPRESSIONS while providing NFTA_RULE_ACTUAL_EXPRS.

I plan to make iptables-nft prefer ACTUAL when fetching rules. Comparing
against EXPRESSIONS is as easy as comparing two rules, but useful rather
as debug-option for sanity checks. The "evil admin" case you mention is
relevant with old user space, which obviously can't do any verification.

> I also wonder if this might cause problems with nftables and implicit
> sets, they are bound to one single lookup expression that, when gone,
> the set is released. Now you will have two expressions pointing to an
> implicit set. Same thing with implicit chains. This might get tricky
> with the transaction interface.

While indeed two lookup expressions will refer to the same anonymous
set, only one of those expressions will ever be in use. There's no way
the kernel would switch between rule variants (or use both at the same
time).

> iptables is rather simple representation (no sets), but nftables is
> more expressive.

That's not true, at least ebtables' among match is implemented using
sets. :)

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Fri, Feb 03, 2023 at 05:21:29PM +0100, Phil Sutter wrote:
[...]
> On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
[...]
> > I also wonder if this might cause problems with nftables and implicit
> > sets, they are bound to one single lookup expression that, when gone,
> > the set is released. Now you will have two expressions pointing to an
> > implicit set. Same thing with implicit chains. This might get tricky
> > with the transaction interface.
> 
> While indeed two lookup expressions will refer to the same anonymous
> set, only one of those expressions will ever be in use. There's no way
> the kernel would switch between rule variants (or use both at the same
> time).

OK, but control plane will reject two lookup expressions that refer to
the same anonymous set.

> > iptables is rather simple representation (no sets), but nftables is
> > more expressive.
> 
> That's not true, at least ebtables' among match is implemented using
> sets. :)

Then better have a look at this implicit set scenario I describe above
because I cannot see how this can work.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Sat, Feb 04, 2023 at 10:41:37AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 03, 2023 at 05:21:29PM +0100, Phil Sutter wrote:
> [...]
> > On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
> [...]
> > > I also wonder if this might cause problems with nftables and implicit
> > > sets, they are bound to one single lookup expression that, when gone,
> > > the set is released. Now you will have two expressions pointing to an
> > > implicit set. Same thing with implicit chains. This might get tricky
> > > with the transaction interface.
> > 
> > While indeed two lookup expressions will refer to the same anonymous
> > set, only one of those expressions will ever be in use. There's no way
> > the kernel would switch between rule variants (or use both at the same
> > time).
> 
> OK, but control plane will reject two lookup expressions that refer to
> the same anonymous set.

Only if it sees the second expression: If NFTA_RULE_ACTUAL_EXPR is
present, the kernel will copy the content of NFTA_RULE_EXPRESSIONS into
a buffer pointed to by nft_rule::dump_expr. It does not inspect the
content apart from nla_policy checking which merely ensures it's a
nested array of elements conforming to nft_expr_policy (i.e., have a
NAME and DATA attribute).

The copied data is touched only by nf_tables_fill_rule_info() which
copies it as-is into the skb. Later, nf_tables_rule_destroy() just frees
the whole blob.

So effectively the kernel doesn't know or care what expressions are
contained in NFTA_RULE_EXPRESSIONS.

> > > iptables is rather simple representation (no sets), but nftables is
> > > more expressive.
> > 
> > That's not true, at least ebtables' among match is implemented using
> > sets. :)
> 
> Then better have a look at this implicit set scenario I describe above
> because I cannot see how this can work.

Sure, I'll give it a try.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Sat, Feb 04, 2023 at 10:00:25PM +0100, Phil Sutter wrote:
> On Sat, Feb 04, 2023 at 10:41:37AM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Feb 03, 2023 at 05:21:29PM +0100, Phil Sutter wrote:
> > [...]
> > > On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
> > [...]
> > > > I also wonder if this might cause problems with nftables and implicit
> > > > sets, they are bound to one single lookup expression that, when gone,
> > > > the set is released. Now you will have two expressions pointing to an
> > > > implicit set. Same thing with implicit chains. This might get tricky
> > > > with the transaction interface.
> > > 
> > > While indeed two lookup expressions will refer to the same anonymous
> > > set, only one of those expressions will ever be in use. There's no way
> > > the kernel would switch between rule variants (or use both at the same
> > > time).
> > 
> > OK, but control plane will reject two lookup expressions that refer to
> > the same anonymous set.
> 
> Only if it sees the second expression: If NFTA_RULE_ACTUAL_EXPR is
> present, the kernel will copy the content of NFTA_RULE_EXPRESSIONS into
> a buffer pointed to by nft_rule::dump_expr. It does not inspect the
> content apart from nla_policy checking which merely ensures it's a
> nested array of elements conforming to nft_expr_policy (i.e., have a
> NAME and DATA attribute).
> 
> The copied data is touched only by nf_tables_fill_rule_info() which
> copies it as-is into the skb. Later, nf_tables_rule_destroy() just frees
> the whole blob.
> 
> So effectively the kernel doesn't know or care what expressions are
> contained in NFTA_RULE_EXPRESSIONS.

Copy should work, sorry I thought you were parsing the expression again.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Mon, Feb 06, 2023 at 10:52:29AM +0100, Pablo Neira Ayuso wrote:
> On Sat, Feb 04, 2023 at 10:00:25PM +0100, Phil Sutter wrote:
> > On Sat, Feb 04, 2023 at 10:41:37AM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Feb 03, 2023 at 05:21:29PM +0100, Phil Sutter wrote:
> > > [...]
> > > > On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
> > > [...]
> > > > > I also wonder if this might cause problems with nftables and implicit
> > > > > sets, they are bound to one single lookup expression that, when gone,
> > > > > the set is released. Now you will have two expressions pointing to an
> > > > > implicit set. Same thing with implicit chains. This might get tricky
> > > > > with the transaction interface.
> > > > 
> > > > While indeed two lookup expressions will refer to the same anonymous
> > > > set, only one of those expressions will ever be in use. There's no way
> > > > the kernel would switch between rule variants (or use both at the same
> > > > time).
> > > 
> > > OK, but control plane will reject two lookup expressions that refer to
> > > the same anonymous set.
> > 
> > Only if it sees the second expression: If NFTA_RULE_ACTUAL_EXPR is
> > present, the kernel will copy the content of NFTA_RULE_EXPRESSIONS into
> > a buffer pointed to by nft_rule::dump_expr. It does not inspect the
> > content apart from nla_policy checking which merely ensures it's a
> > nested array of elements conforming to nft_expr_policy (i.e., have a
> > NAME and DATA attribute).
> > 
> > The copied data is touched only by nf_tables_fill_rule_info() which
> > copies it as-is into the skb. Later, nf_tables_rule_destroy() just frees
> > the whole blob.
> > 
> > So effectively the kernel doesn't know or care what expressions are
> > contained in NFTA_RULE_EXPRESSIONS.
> 
> Copy should work, sorry I thought you were parsing the expression again.

If you are happy with this, then let's place it in nf-next?

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Tue, Feb 07, 2023 at 11:43:28AM +0100, Pablo Neira Ayuso wrote:
> On Mon, Feb 06, 2023 at 10:52:29AM +0100, Pablo Neira Ayuso wrote:
> > On Sat, Feb 04, 2023 at 10:00:25PM +0100, Phil Sutter wrote:
> > > On Sat, Feb 04, 2023 at 10:41:37AM +0100, Pablo Neira Ayuso wrote:
> > > > On Fri, Feb 03, 2023 at 05:21:29PM +0100, Phil Sutter wrote:
> > > > [...]
> > > > > On Fri, Feb 03, 2023 at 04:32:01PM +0100, Pablo Neira Ayuso wrote:
> > > > [...]
> > > > > > I also wonder if this might cause problems with nftables and implicit
> > > > > > sets, they are bound to one single lookup expression that, when gone,
> > > > > > the set is released. Now you will have two expressions pointing to an
> > > > > > implicit set. Same thing with implicit chains. This might get tricky
> > > > > > with the transaction interface.
> > > > > 
> > > > > While indeed two lookup expressions will refer to the same anonymous
> > > > > set, only one of those expressions will ever be in use. There's no way
> > > > > the kernel would switch between rule variants (or use both at the same
> > > > > time).
> > > > 
> > > > OK, but control plane will reject two lookup expressions that refer to
> > > > the same anonymous set.
> > > 
> > > Only if it sees the second expression: If NFTA_RULE_ACTUAL_EXPR is
> > > present, the kernel will copy the content of NFTA_RULE_EXPRESSIONS into
> > > a buffer pointed to by nft_rule::dump_expr. It does not inspect the
> > > content apart from nla_policy checking which merely ensures it's a
> > > nested array of elements conforming to nft_expr_policy (i.e., have a
> > > NAME and DATA attribute).
> > > 
> > > The copied data is touched only by nf_tables_fill_rule_info() which
> > > copies it as-is into the skb. Later, nf_tables_rule_destroy() just frees
> > > the whole blob.
> > > 
> > > So effectively the kernel doesn't know or care what expressions are
> > > contained in NFTA_RULE_EXPRESSIONS.
> > 
> > Copy should work, sorry I thought you were parsing the expression again.
> 
> If you are happy with this, then let's place it in nf-next?

Yes, please! I'll finish user space this week. :)

Thanks, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Tue, Feb 07, 2023 at 11:56:53AM +0100, Phil Sutter wrote:
[...]
> Yes, please! I'll finish user space this week. :)

Famous last words. :(

I realized anonymous sets are indeed a problem, and I'm not sure how it
could be solved. I missed the fact that with lookup expressions one has
to run the init() callback to convert their per-batch set ID into the
kernel-defined set name. So the simple "copy and return nla" approach is
not sufficient.

Initializing all of the dump-only expressions though causes other
unwanted side-effects, like e.g. duplicated chain use-counters.

One could ban lookup from being used in dump-only expressions. Right
now, only ebtables' among match requires it.

To still allow for ebtables-nft to use the compat interface, among match
could be rewritten to use the legacy extension in-kernel. This doesn't
solve the original problem though, because old ebtables-nft versions
can't parse a match expression containing among extension.

Another option that might work is to parse the dump-only expressions in
nf_tables_newrule(), dump them into an skb, drop them again and extract
the skb's buffer for later.

Do you have a better idea perhaps? I'm a bit clueless how to proceed
further right now. :(

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Thu, Feb 16, 2023 at 11:55:28AM +0100, Phil Sutter wrote:
> On Tue, Feb 07, 2023 at 11:56:53AM +0100, Phil Sutter wrote:
> [...]
> > Yes, please! I'll finish user space this week. :)
> 
> Famous last words. :(
> 
> I realized anonymous sets are indeed a problem, and I'm not sure how it
> could be solved. I missed the fact that with lookup expressions one has
> to run the init() callback to convert their per-batch set ID into the
> kernel-defined set name. So the simple "copy and return nla" approach is
> not sufficient.
> 
> Initializing all of the dump-only expressions though causes other
> unwanted side-effects, like e.g. duplicated chain use-counters.
> 
> One could ban lookup from being used in dump-only expressions. Right
> now, only ebtables' among match requires it.
> 
> To still allow for ebtables-nft to use the compat interface, among match
> could be rewritten to use the legacy extension in-kernel. This doesn't
> solve the original problem though, because old ebtables-nft versions
> can't parse a match expression containing among extension.
> 
> Another option that might work is to parse the dump-only expressions in
> nf_tables_newrule(), dump them into an skb, drop them again and extract
> the skb's buffer for later.
> 
> Do you have a better idea perhaps? I'm a bit clueless how to proceed
> further right now. :(

I'll drop the patch from nf-next and we take more time to think how to
solve this.

This problem is interesting, but it is difficult!

Does this work for you?

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Thu, Feb 16, 2023 at 12:29:30PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Feb 16, 2023 at 11:55:28AM +0100, Phil Sutter wrote:
> > On Tue, Feb 07, 2023 at 11:56:53AM +0100, Phil Sutter wrote:
> > [...]
> > > Yes, please! I'll finish user space this week. :)
> > 
> > Famous last words. :(
> > 
> > I realized anonymous sets are indeed a problem, and I'm not sure how it
> > could be solved. I missed the fact that with lookup expressions one has
> > to run the init() callback to convert their per-batch set ID into the
> > kernel-defined set name. So the simple "copy and return nla" approach is
> > not sufficient.
> > 
> > Initializing all of the dump-only expressions though causes other
> > unwanted side-effects, like e.g. duplicated chain use-counters.
> > 
> > One could ban lookup from being used in dump-only expressions. Right
> > now, only ebtables' among match requires it.
> > 
> > To still allow for ebtables-nft to use the compat interface, among match
> > could be rewritten to use the legacy extension in-kernel. This doesn't
> > solve the original problem though, because old ebtables-nft versions
> > can't parse a match expression containing among extension.
> > 
> > Another option that might work is to parse the dump-only expressions in
> > nf_tables_newrule(), dump them into an skb, drop them again and extract
> > the skb's buffer for later.
> > 
> > Do you have a better idea perhaps? I'm a bit clueless how to proceed
> > further right now. :(
> 
> I'll drop the patch from nf-next and we take more time to think how to
> solve this.

ACK!

> This problem is interesting, but it is difficult!

Yes, it is. Maybe a feasible solution is to scan through the dump-only
expression nla and update any lookup ones manually. Pretty ugly though,
because it breaks the attribute encapsulation in expressions.

> Does this work for you?

Yes, thanks!

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

Hi,

On Thu, Feb 16, 2023 at 01:05:26PM +0100, Phil Sutter wrote:
> On Thu, Feb 16, 2023 at 12:29:30PM +0100, Pablo Neira Ayuso wrote:
> > On Thu, Feb 16, 2023 at 11:55:28AM +0100, Phil Sutter wrote:
> > > On Tue, Feb 07, 2023 at 11:56:53AM +0100, Phil Sutter wrote:
> > > [...]
> > > > Yes, please! I'll finish user space this week. :)
> > > 
> > > Famous last words. :(
> > > 
> > > I realized anonymous sets are indeed a problem, and I'm not sure how it
> > > could be solved. I missed the fact that with lookup expressions one has
> > > to run the init() callback to convert their per-batch set ID into the
> > > kernel-defined set name. So the simple "copy and return nla" approach is
> > > not sufficient.
> > > 
> > > Initializing all of the dump-only expressions though causes other
> > > unwanted side-effects, like e.g. duplicated chain use-counters.
> > > 
> > > One could ban lookup from being used in dump-only expressions. Right
> > > now, only ebtables' among match requires it.
> > > 
> > > To still allow for ebtables-nft to use the compat interface, among match
> > > could be rewritten to use the legacy extension in-kernel. This doesn't
> > > solve the original problem though, because old ebtables-nft versions
> > > can't parse a match expression containing among extension.
> > > 
> > > Another option that might work is to parse the dump-only expressions in
> > > nf_tables_newrule(), dump them into an skb, drop them again and extract
> > > the skb's buffer for later.
> > > 
> > > Do you have a better idea perhaps? I'm a bit clueless how to proceed
> > > further right now. :(
> > 
> > I'll drop the patch from nf-next and we take more time to think how to
> > solve this.
> 
> ACK!
> 
> > This problem is interesting, but it is difficult!
> 
> Yes, it is. Maybe a feasible solution is to scan through the dump-only
> expression nla and update any lookup ones manually. Pretty ugly though,
> because it breaks the attribute encapsulation in expressions.

My proposal:

- Add support for cookies, this is an identifier that the user can
  specify when the object is created, this is allocated by the user.
  We already discussed this in the past for different purpose. The idea
  would be to add a _COOKIE attribute to the objects, which is dumped
  via netlink.
- Add the alternative compat representation to the userdata, use the
  cookie identifier to refer to the anonymous set. By the time you
  create the anonymous set, you can already

With this approach, you add cookie support - which is something that
has been already discussed in the past - and you can use it from the
userdata to refer to the anonymous set.

If you fall back to the compat representation, then you look at the
userdata and, if there is a cookie reference, you can fetch the object
accordingly and put all pieces together to print the rule.

You could possibly make this without kernel updates? Add an internal
cookie field in userdata, that is included in the anonymous set. Then,
from the rule userdata, you refer to the internal cookie that refers
to the anonymous set. In such case, you can implement all what you
need from userspace, without kernel updates, to deal with this "forward
compatibility" requirement for the containers case.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

Hi Pablo,

On Wed, Apr 26, 2023 at 09:58:44PM +0200, Pablo Neira Ayuso wrote:
[...]
> My proposal:

Thanks for returning to this. Your approach requires to define a minimum
version from which on forward-compat is guaranteed. I was trying to
avoid this requirement though so things would work for "unknown user
space".

Currently, the only offending extension is ebt_among since it doesn't
exist (and never did) in non-native form. If I implement among extension
parsing (even in non-functional form), my original approach would work.
This also means having a minimum version for full compat, but it affects
ebtables (actually, use of ebt_among) only.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Thu, Apr 27, 2023 at 12:57:30PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, Apr 26, 2023 at 09:58:44PM +0200, Pablo Neira Ayuso wrote:
> [...]
> > My proposal:
> 
> Thanks for returning to this. Your approach requires to define a minimum
> version from which on forward-compat is guaranteed. I was trying to
> avoid this requirement though so things would work for "unknown user
> space".

You also require a kernel that supports your approach.

> Currently, the only offending extension is ebt_among since it doesn't
> exist (and never did) in non-native form. If I implement among extension
> parsing (even in non-functional form), my original approach would work.
> This also means having a minimum version for full compat, but it affects
> ebtables (actually, use of ebt_among) only.

Yes, but this is fully user data, kernel really does not need to do
anything with this alternative representation, which is what I do not
like from you proposal.

I really think userdata is the place to deal with this.

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Thu, Apr 27, 2023 at 01:01:55PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 27, 2023 at 12:57:30PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, Apr 26, 2023 at 09:58:44PM +0200, Pablo Neira Ayuso wrote:
> > [...]
> > > My proposal:
> > 
> > Thanks for returning to this. Your approach requires to define a minimum
> > version from which on forward-compat is guaranteed. I was trying to
> > avoid this requirement though so things would work for "unknown user
> > space".
> 
> You also require a kernel that supports your approach.

Sure. But in the described use-case, anything but old user space (i.e.,
container content) is under control.

> > Currently, the only offending extension is ebt_among since it doesn't
> > exist (and never did) in non-native form. If I implement among extension
> > parsing (even in non-functional form), my original approach would work.
> > This also means having a minimum version for full compat, but it affects
> > ebtables (actually, use of ebt_among) only.
> 
> Yes, but this is fully user data, kernel really does not need to do
> anything with this alternative representation, which is what I do not
> like from you proposal.

OK.

> I really think userdata is the place to deal with this.

Having to touch old user space is not a good solution for the given
use-case. If kernel modification is a no-go, I'd rather introduce a
"compat mode" in iptables-nft which causes rule creation in the most
compatible form. This might impact run-time performance but is much
simpler to implement and maintain.

Cheers, Phil

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Pablo Neira Ayuso]

On Thu, Apr 27, 2023 at 01:33:13PM +0200, Phil Sutter wrote:
> On Thu, Apr 27, 2023 at 01:01:55PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Apr 27, 2023 at 12:57:30PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Wed, Apr 26, 2023 at 09:58:44PM +0200, Pablo Neira Ayuso wrote:
> > > [...]
> > > > My proposal:
> > > 
> > > Thanks for returning to this. Your approach requires to define a minimum
> > > version from which on forward-compat is guaranteed. I was trying to
> > > avoid this requirement though so things would work for "unknown user
> > > space".
> > 
> > You also require a kernel that supports your approach.
> 
> Sure. But in the described use-case, anything but old user space (i.e.,
> container content) is under control.

This is a "forward compatibility" mechanism, we can do nothing about
the past, but prepare to handle this scenario better in the future.

This problem has been always there, and we already discussed that it
affects other existing utilities and interfaces in the kernel,
including iptables legacy.

> > > Currently, the only offending extension is ebt_among since it doesn't
> > > exist (and never did) in non-native form. If I implement among extension
> > > parsing (even in non-functional form), my original approach would work.
> > > This also means having a minimum version for full compat, but it affects
> > > ebtables (actually, use of ebt_among) only.
> > 
> > Yes, but this is fully user data, kernel really does not need to do
> > anything with this alternative representation, which is what I do not
> > like from you proposal.
> 
> OK.
> 
> > I really think userdata is the place to deal with this.
> 
> Having to touch old user space is not a good solution for the given
> use-case. If kernel modification is a no-go, I'd rather introduce a
> "compat mode" in iptables-nft which causes rule creation in the most
> compatible form. This might impact run-time performance but is much
> simpler to implement and maintain.

This introduces conditional bytecode generation, how will you enable
this?

Re: [nf-next PATCH v2] netfilter: nf_tables: Introduce NFTA_RULE_ACTUAL_EXPR

[Phil Sutter]

On Thu, Apr 27, 2023 at 03:07:24PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 27, 2023 at 01:33:13PM +0200, Phil Sutter wrote:
> > On Thu, Apr 27, 2023 at 01:01:55PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Apr 27, 2023 at 12:57:30PM +0200, Phil Sutter wrote:
> > > > Hi Pablo,
> > > > 
> > > > On Wed, Apr 26, 2023 at 09:58:44PM +0200, Pablo Neira Ayuso wrote:
> > > > [...]
> > > > > My proposal:
> > > > 
> > > > Thanks for returning to this. Your approach requires to define a minimum
> > > > version from which on forward-compat is guaranteed. I was trying to
> > > > avoid this requirement though so things would work for "unknown user
> > > > space".
> > > 
> > > You also require a kernel that supports your approach.
> > 
> > Sure. But in the described use-case, anything but old user space (i.e.,
> > container content) is under control.
> 
> This is a "forward compatibility" mechanism, we can do nothing about
> the past, but prepare to handle this scenario better in the future.
> 
> This problem has been always there, and we already discussed that it
> affects other existing utilities and interfaces in the kernel,
> including iptables legacy.

Yes, sure. If iptables-legacy moved at the pace iptables-nft did and
containers were a thing back then, the same situation had arisen. I
don't even see us in charge to solve the problem, other tools lack
forward compatibility, too. And I have not seen any guarantees about
mixed use of different tool versions. The only reason I see for
iptables-nft to attempt such a guarantee is its perspective of replacing
itself internally with native nftables expressions, in consequence the
tendency of each new version to break compatibility for the previous
one.

Introducing a mode which creates a compatible ruleset for users
requiring it should be enough from my point of view. Sure, one may argue
it hinders future deprecation of xtables extensions in kernel. But the
active use of older versions of iptables-nft does that already.

> > > > Currently, the only offending extension is ebt_among since it doesn't
> > > > exist (and never did) in non-native form. If I implement among extension
> > > > parsing (even in non-functional form), my original approach would work.
> > > > This also means having a minimum version for full compat, but it affects
> > > > ebtables (actually, use of ebt_among) only.
> > > 
> > > Yes, but this is fully user data, kernel really does not need to do
> > > anything with this alternative representation, which is what I do not
> > > like from you proposal.
> > 
> > OK.
> > 
> > > I really think userdata is the place to deal with this.
> > 
> > Having to touch old user space is not a good solution for the given
> > use-case. If kernel modification is a no-go, I'd rather introduce a
> > "compat mode" in iptables-nft which causes rule creation in the most
> > compatible form. This might impact run-time performance but is much
> > simpler to implement and maintain.
> 
> This introduces conditional bytecode generation, how will you enable
> this?

Commandline (or input file) parser creates match and target objects
which add_match() may or may not turn into native code. In compat mode,
one could always allocate a "match" expression and call __add_match().

In turn, nftnl_rule parser creates match and target objects from native
expressions (if supported). Upon encountering a "match" object, it
merely does an extension lookup and copies the payload into the
allocated object.

So compat mode is a flag for iptables-nft-restore and 'iptables-nft -A',
the reverse path works automatically.

Cheers, Phil