[1/2,nft,v2] src: osf: add ttl option support

Message ID 20180929101518.843-1-ffmancera@riseup.net
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [1/2,nft,v2] src: osf: add ttl option support
Related show

Commit Message

Fernando Fernandez Mancera Sept. 29, 2018, 10:15 a.m.
Add support for ttl option in "osf" expression. Example:

table ip foo {
	chain bar {
		type filter hook input priority filter; policy accept;
		osf ttl nocheck name "Linux"
	}
}

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
v1: initial patch
v2: use "ttl-global" or "ttl-nocheck" instead of "1" or "2"
---
 include/expression.h                |  4 ++++
 include/linux/netfilter/nf_tables.h |  2 ++
 include/osf.h                       |  2 +-
 src/netlink_delinearize.c           |  5 ++++-
 src/netlink_linearize.c             |  1 +
 src/osf.c                           | 26 ++++++++++++++++++++++++--
 src/parser_bison.y                  | 17 +++++++++++++++--
 7 files changed, 51 insertions(+), 6 deletions(-)

Comments

Pablo Neira Ayuso Oct. 15, 2018, 12:47 p.m. | #1
Please send a v3 including tests/py. More comments below.

On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote:
> Add support for ttl option in "osf" expression. Example:
> 
> table ip foo {
> 	chain bar {
> 		type filter hook input priority filter; policy accept;
> 		osf ttl nocheck name "Linux"

Listing and output should match, ie. what you list should work with
nft -f, see below.

> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 831090b..a7ec858 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *);
>  %type <val>			fib_tuple	fib_result	fib_flag
>  
>  %type <expr>			osf_expr
> +%type <val>			osf_ttl
>  %destructor { expr_free($$); }	osf_expr
>  
>  %type <val>			markup_format
> @@ -3112,9 +3113,21 @@ fib_tuple		:  	fib_flag	DOT	fib_tuple
>  			|	fib_flag
>  			;
>  
> -osf_expr		:	OSF	NAME
> +osf_expr		:	OSF	osf_ttl		NAME
>  			{
> -				$$ = osf_expr_alloc(&@$);
> +				$$ = osf_expr_alloc(&@$, $2);
> +			}
> +			;
> +
> +osf_ttl			:	/* empty */	{ $$ = 0; }
> +			|	STRING
> +			{
> +				if (!strcmp($1, "ttl-global"))

This should be "global". But I would suggest you rename this to "loose".

> +					$$ = 1;

Can we use NFT_OSF_* definitions, instead of magic number?

> +				else if (!strcmp($1, "ttl-nocheck"))

This should be "nocheck". But I'd suggest you rename this to "skip"

> +					$$ = 2;

Same here, avoid magic number, use definition.

> +				else
> +					$$ = 3;

Same thing.

>  			}
>  			;
> diff --git a/src/osf.c b/src/osf.c
> index 85c9573..c7dd25f 100644
> --- a/src/osf.c
> +++ b/src/osf.c
> @@ -5,13 +5,32 @@
>  #include <osf.h>
>  #include <json.h>
>  
> +static const char *osf_ttl_int_to_str(const uint8_t ttl)
> +{
> +	if (ttl == 1)
> +		return "ttl global";
> +	else if (ttl == 2)
> +		return "ttl nocheck";
> +
> +	return "";
> +}
> +
>  static void osf_expr_print(const struct expr *expr, struct output_ctx *octx)
>  {
> -	nft_print(octx, "osf name");
> +	const char *ttl_str;
> +
> +	ttl_str = osf_ttl_int_to_str(expr->osf.ttl);
> +	nft_print(octx, "osf %s name", ttl_str);
>  }
>  
>  static void osf_expr_clone(struct expr *new, const struct expr *expr)
>  {
> +	new->osf.ttl = expr->osf.ttl;
> +}
> +
> +static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2)
> +{
> +	return e1->osf.ttl == e2->osf.ttl;
>  }
>  
>  static const struct expr_ops osf_expr_ops = {
> @@ -19,10 +38,11 @@ static const struct expr_ops osf_expr_ops = {
>  	.name		= "osf",
>  	.print		= osf_expr_print,
>  	.clone		= osf_expr_clone,
> +	.cmp		= osf_expr_cmp,
>  	.json		= osf_expr_json,

BTW, could you extend json to support 'ttl' too?
Fernando Fernandez Mancera Oct. 17, 2018, 6:41 a.m. | #2
On 10/15/18 2:47 PM, Pablo Neira Ayuso wrote:
> Please send a v3 including tests/py. More comments below.
> 
> On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote:
>> Add support for ttl option in "osf" expression. Example:
>>
>> table ip foo {
>> 	chain bar {
>> 		type filter hook input priority filter; policy accept;
>> 		osf ttl nocheck name "Linux"
> 
> Listing and output should match, ie. what you list should work with
> nft -f, see below.
> 
>> diff --git a/src/parser_bison.y b/src/parser_bison.y
>> index 831090b..a7ec858 100644
>> --- a/src/parser_bison.y
>> +++ b/src/parser_bison.y
>> @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *);
>>   %type <val>			fib_tuple	fib_result	fib_flag
>>   
>>   %type <expr>			osf_expr
>> +%type <val>			osf_ttl
>>   %destructor { expr_free($$); }	osf_expr
>>   
>>   %type <val>			markup_format
>> @@ -3112,9 +3113,21 @@ fib_tuple		:  	fib_flag	DOT	fib_tuple
>>   			|	fib_flag
>>   			;
>>   
>> -osf_expr		:	OSF	NAME
>> +osf_expr		:	OSF	osf_ttl		NAME
>>   			{
>> -				$$ = osf_expr_alloc(&@$);
>> +				$$ = osf_expr_alloc(&@$, $2);
>> +			}
>> +			;
>> +
>> +osf_ttl			:	/* empty */	{ $$ = 0; }
>> +			|	STRING
>> +			{
>> +				if (!strcmp($1, "ttl-global"))
> 
> This should be "global". But I would suggest you rename this to "loose".
> 
>> +					$$ = 1;
> 
> Can we use NFT_OSF_* definitions, instead of magic number?
> 
>> +				else if (!strcmp($1, "ttl-nocheck"))
> 
> This should be "nocheck". But I'd suggest you rename this to "skip"
> 
>> +					$$ = 2;
> 
> Same here, avoid magic number, use definition.
> 
>> +				else
>> +					$$ = 3;
> 
> Same thing.
> 
>>   			}
>>   			;
>> diff --git a/src/osf.c b/src/osf.c
>> index 85c9573..c7dd25f 100644
>> --- a/src/osf.c
>> +++ b/src/osf.c
>> @@ -5,13 +5,32 @@
>>   #include <osf.h>
>>   #include <json.h>
>>   
>> +static const char *osf_ttl_int_to_str(const uint8_t ttl)
>> +{
>> +	if (ttl == 1)
>> +		return "ttl global";
>> +	else if (ttl == 2)
>> +		return "ttl nocheck";
>> +
>> +	return "";
>> +}
>> +
>>   static void osf_expr_print(const struct expr *expr, struct output_ctx *octx)
>>   {
>> -	nft_print(octx, "osf name");
>> +	const char *ttl_str;
>> +
>> +	ttl_str = osf_ttl_int_to_str(expr->osf.ttl);
>> +	nft_print(octx, "osf %s name", ttl_str);
>>   }
>>   
>>   static void osf_expr_clone(struct expr *new, const struct expr *expr)
>>   {
>> +	new->osf.ttl = expr->osf.ttl;
>> +}
>> +
>> +static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2)
>> +{
>> +	return e1->osf.ttl == e2->osf.ttl;
>>   }
>>   
>>   static const struct expr_ops osf_expr_ops = {
>> @@ -19,10 +38,11 @@ static const struct expr_ops osf_expr_ops = {
>>   	.name		= "osf",
>>   	.print		= osf_expr_print,
>>   	.clone		= osf_expr_clone,
>> +	.cmp		= osf_expr_cmp,
>>   	.json		= osf_expr_json,
> 
> BTW, could you extend json to support 'ttl' too?
>

Hi Pablo, thanks for this review.

All changes seem fine to me :-)
Fernando Fernandez Mancera Oct. 22, 2018, 3:35 p.m. | #3
Comments below.

On 10/15/18 2:47 PM, Pablo Neira Ayuso wrote:
> Please send a v3 including tests/py. More comments below.
> 
> On Sat, Sep 29, 2018 at 12:15:17PM +0200, Fernando Fernandez Mancera wrote:
>> Add support for ttl option in "osf" expression. Example:
>>
>> table ip foo {
>> 	chain bar {
>> 		type filter hook input priority filter; policy accept;
>> 		osf ttl nocheck name "Linux"
> 
> Listing and output should match, ie. what you list should work with
> nft -f, see below.
> 
>> diff --git a/src/parser_bison.y b/src/parser_bison.y
>> index 831090b..a7ec858 100644
>> --- a/src/parser_bison.y
>> +++ b/src/parser_bison.y
>> @@ -739,6 +739,7 @@ int nft_lex(void *, void *, void *);
>>   %type <val>			fib_tuple	fib_result	fib_flag
>>   
>>   %type <expr>			osf_expr
>> +%type <val>			osf_ttl
>>   %destructor { expr_free($$); }	osf_expr
>>   
>>   %type <val>			markup_format
>> @@ -3112,9 +3113,21 @@ fib_tuple		:  	fib_flag	DOT	fib_tuple
>>   			|	fib_flag
>>   			;
>>   
>> -osf_expr		:	OSF	NAME
>> +osf_expr		:	OSF	osf_ttl		NAME
>>   			{
>> -				$$ = osf_expr_alloc(&@$);
>> +				$$ = osf_expr_alloc(&@$, $2);
>> +			}
>> +			;
>> +
>> +osf_ttl			:	/* empty */	{ $$ = 0; }
>> +			|	STRING
>> +			{
>> +				if (!strcmp($1, "ttl-global"))
> 
> This should be "global". But I would suggest you rename this to "loose".
> 
>> +					$$ = 1;
> 
> Can we use NFT_OSF_* definitions, instead of magic number?
> 
>> +				else if (!strcmp($1, "ttl-nocheck"))
> 
> This should be "nocheck". But I'd suggest you rename this to "skip"
> 
>> +					$$ = 2;
> 
> Same here, avoid magic number, use definition.
> 
>> +				else
>> +					$$ = 3;
> 
> Same thing.
>

I am going to add the necessary NFT_OSF_* definitions in the nf_tables.h 
header. The options format is "ttl-*" because "global" or "loose" could 
be confusing. Do you prefer the option without "ttl-"?

Thanks.
Pablo Neira Ayuso Oct. 22, 2018, 6:38 p.m. | #4
On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera wrote:
> I am going to add the necessary NFT_OSF_* definitions in the nf_tables.h

Just add a copy of nf_osf.h to nftables tree. We cannot mangle
nf_tables.h, it's a copy from the original header to ensure sources
compile with any kernel version.

> header. The options format is "ttl-*" because "global" or "loose" could be
> confusing. Do you prefer the option without "ttl-"?

Yes, the ttl- is not needed.

Make sure what you list via 'nft list ruleset > file' can be loaded
via 'nft -f file'
Fernando Fernandez Mancera Oct. 22, 2018, 7:38 p.m. | #5
El 22 de octubre de 2018 20:38:13 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió:
>On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera
>wrote:
>> I am going to add the necessary NFT_OSF_* definitions in the
>nf_tables.h
>
>Just add a copy of nf_osf.h to nftables tree. We cannot mangle
>nf_tables.h, it's a copy from the original header to ensure sources
>compile with any kernel version.
>
>> header. The options format is "ttl-*" because "global" or "loose"
>could be
>> confusing. Do you prefer the option without "ttl-"?
>
>Yes, the ttl- is not needed.
>
>Make sure what you list via 'nft list ruleset > file' can be loaded
>via 'nft -f file'

The patch is already done but tests/py is giving me errors when using ct mark and maps with osf. Should I send it?

I am trying to solve this anyway.

Thanks :-)
Pablo Neira Ayuso Oct. 22, 2018, 8:14 p.m. | #6
On Mon, Oct 22, 2018 at 09:38:31PM +0200, Fernando Fernandez Mancera wrote:
> El 22 de octubre de 2018 20:38:13 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió:
> >On Mon, Oct 22, 2018 at 05:35:42PM +0200, Fernando Fernandez Mancera
> >wrote:
> >> I am going to add the necessary NFT_OSF_* definitions in the
> >nf_tables.h
> >
> >Just add a copy of nf_osf.h to nftables tree. We cannot mangle
> >nf_tables.h, it's a copy from the original header to ensure sources
> >compile with any kernel version.
> >
> >> header. The options format is "ttl-*" because "global" or "loose"
> >could be
> >> confusing. Do you prefer the option without "ttl-"?
> >
> >Yes, the ttl- is not needed.
> >
> >Make sure what you list via 'nft list ruleset > file' can be loaded
> >via 'nft -f file'
> 
> The patch is already done but tests/py is giving me errors when
> using ct mark and maps with osf. Should I send it?

You can send a preview / RFC.

Patch

diff --git a/include/expression.h b/include/expression.h
index fb52abf..a794128 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -345,6 +345,10 @@  struct expr {
 			uint8_t		direction;
 			uint8_t		spnum;
 		} xfrm;
+		struct {
+			/* EXPR_OSF */
+			uint8_t			ttl;
+		} osf;
 	};
 };
 
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 169c2ab..1003942 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -939,10 +939,12 @@  enum nft_socket_keys {
  * enum nft_osf_attributes - nf_tables osf expression netlink attributes
  *
  * @NFTA_OSF_DREG: destination register (NLA_U32: nft_registers)
+ * @NFTA_OSF_TTL: Value of the TTL osf option (NLA_U8)
  */
 enum nft_osf_attributes {
 	NFTA_OSF_UNSPEC,
 	NFTA_OSF_DREG,
+	NFTA_OSF_TTL,
 	__NFTA_OSF_MAX
 };
 #define NFT_OSF_MAX		(__NFTA_OSF_MAX - 1)
diff --git a/include/osf.h b/include/osf.h
index 54cdd4a..23ea34d 100644
--- a/include/osf.h
+++ b/include/osf.h
@@ -1,7 +1,7 @@ 
 #ifndef NFTABLES_OSF_H
 #define NFTABLES_OSF_H
 
-struct expr *osf_expr_alloc(const struct location *loc);
+struct expr *osf_expr_alloc(const struct location *loc, const uint8_t ttl);
 
 extern int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, int del);
 
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 0a6ebe0..d474893 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -655,8 +655,11 @@  static void netlink_parse_osf(struct netlink_parse_ctx *ctx,
 {
 	enum nft_registers dreg;
 	struct expr *expr;
+	uint8_t ttl;
+
+	ttl = nftnl_expr_get_u8(nle, NFTNL_EXPR_OSF_TTL);
+	expr = osf_expr_alloc(loc, ttl);
 
-	expr = osf_expr_alloc(loc);
 	dreg = netlink_parse_register(nle, NFTNL_EXPR_OSF_DREG);
 	netlink_set_register(ctx, dreg, expr);
 }
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 0ac51bd..0c8f5fe 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -227,6 +227,7 @@  static void netlink_gen_osf(struct netlink_linearize_ctx *ctx,
 
 	nle = alloc_nft_expr("osf");
 	netlink_put_register(nle, NFTNL_EXPR_OSF_DREG, dreg);
+	nftnl_expr_set_u8(nle, NFTNL_EXPR_OSF_TTL, expr->osf.ttl);
 	nftnl_rule_add_expr(ctx->nlr, nle);
 }
 
diff --git a/src/osf.c b/src/osf.c
index 85c9573..c7dd25f 100644
--- a/src/osf.c
+++ b/src/osf.c
@@ -5,13 +5,32 @@ 
 #include <osf.h>
 #include <json.h>
 
+static const char *osf_ttl_int_to_str(const uint8_t ttl)
+{
+	if (ttl == 1)
+		return "ttl global";
+	else if (ttl == 2)
+		return "ttl nocheck";
+
+	return "";
+}
+
 static void osf_expr_print(const struct expr *expr, struct output_ctx *octx)
 {
-	nft_print(octx, "osf name");
+	const char *ttl_str;
+
+	ttl_str = osf_ttl_int_to_str(expr->osf.ttl);
+	nft_print(octx, "osf %s name", ttl_str);
 }
 
 static void osf_expr_clone(struct expr *new, const struct expr *expr)
 {
+	new->osf.ttl = expr->osf.ttl;
+}
+
+static bool osf_expr_cmp(const struct expr *e1, const struct expr *e2)
+{
+	return e1->osf.ttl == e2->osf.ttl;
 }
 
 static const struct expr_ops osf_expr_ops = {
@@ -19,10 +38,11 @@  static const struct expr_ops osf_expr_ops = {
 	.name		= "osf",
 	.print		= osf_expr_print,
 	.clone		= osf_expr_clone,
+	.cmp		= osf_expr_cmp,
 	.json		= osf_expr_json,
 };
 
-struct expr *osf_expr_alloc(const struct location *loc)
+struct expr *osf_expr_alloc(const struct location *loc, const uint8_t ttl)
 {
 	unsigned int len = NFT_OSF_MAXGENRELEN * BITS_PER_BYTE;
 	const struct datatype *type = &string_type;
@@ -31,5 +51,7 @@  struct expr *osf_expr_alloc(const struct location *loc)
 	expr = expr_alloc(loc, &osf_expr_ops, type,
 			  BYTEORDER_HOST_ENDIAN, len);
 
+	expr->osf.ttl = ttl;
+
 	return expr;
 }
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 831090b..a7ec858 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -739,6 +739,7 @@  int nft_lex(void *, void *, void *);
 %type <val>			fib_tuple	fib_result	fib_flag
 
 %type <expr>			osf_expr
+%type <val>			osf_ttl
 %destructor { expr_free($$); }	osf_expr
 
 %type <val>			markup_format
@@ -3112,9 +3113,21 @@  fib_tuple		:  	fib_flag	DOT	fib_tuple
 			|	fib_flag
 			;
 
-osf_expr		:	OSF	NAME
+osf_expr		:	OSF	osf_ttl		NAME
 			{
-				$$ = osf_expr_alloc(&@$);
+				$$ = osf_expr_alloc(&@$, $2);
+			}
+			;
+
+osf_ttl			:	/* empty */	{ $$ = 0; }
+			|	STRING
+			{
+				if (!strcmp($1, "ttl-global"))
+					$$ = 1;
+				else if (!strcmp($1, "ttl-nocheck"))
+					$$ = 2;
+				else
+					$$ = 3;
 			}
 			;