nft parser and problems with icmp type names (redirect and param-problem)

nft parser and problems with icmp type names (redirect and param-problem)

[Alexander Holler]

Hello,

are the problems with some named icmp types known?

I'm talking about

host ~ # nft add rule ip6 filter input icmpv6 type { param-problem } accept
<cmdline>:1:41-53: Error: syntax error, unexpected param-problem
add rule ip6 filter input icmpv6 type { param-problem } accept
                                         ^^^^^^^^^^^^^
host ~ # nft add rule filter input icmp type { redirect } accept
<cmdline>:1:35-42: Error: syntax error, unexpected redirect
add rule filter input icmp type { redirect } accept

Having had a quick look the source, the first one seems to come from the 
fact that "param-problem" is a token as well as an icmpv6-type name.

I haven't looked at the second problem, but I assume it's similiar, 
because "redirect" is now a token too.

Both problems can be cirumvented by using their code instead of name (4 
or 5), but then another problem will arise when trying to save/restore a 
nft ruleset.

E.g. if you call

nft add rule filter input icmp type { 5 } accept

nft list table filter

will show it with the type name, so restoring will fail.


As I'm not familiar with parser, I thought it might be a good idea to 
ask here before I start to think about trying to solve the problem myself.


Regards,

Alexander Holler

Re: nft parser and problems with icmp type names (redirect and param-problem)

[Alexander Holler]

Am 01.04.2015 um 09:58 schrieb Alexander Holler:
> Hello,
>
> are the problems with some named icmp types known?
>
> I'm talking about
>
> host ~ # nft add rule ip6 filter input icmpv6 type { param-problem } accept
> <cmdline>:1:41-53: Error: syntax error, unexpected param-problem
> add rule ip6 filter input icmpv6 type { param-problem } accept
>                                          ^^^^^^^^^^^^^
> host ~ # nft add rule filter input icmp type { redirect } accept
> <cmdline>:1:35-42: Error: syntax error, unexpected redirect
> add rule filter input icmp type { redirect } accept

This message is basically to get Eric Leblond on board, who seems to 
have written the stuff which made it possible to use icmp type names.


But to add something useful to this message too:

Having digged a bit further I see two solutions.

- Change all the icmp type names to not get in conflict with tokens 
(keywords), e.g. by prefixing them with "icmp_" or "icmpv6_" like 
"icmp_redirect". That would be a clean and straight forward solution. 
Unfortunately it would mean old (icmp type) rules won't work and 
personally I think the longer names would be a bit unhandy to use.

- Add context dependency to the parser. The relevant part in the bison 
manual would be the chapter "Handling Context Dependencies": 
http://www.chemie.fu-berlin.de/chemnet/use/info/bison/bison_10.html

Personally I would prefer the second solution, also it means the code 
would become a bit more complicated.

Any comments which solution would be prefered by other people?

Regards,

Alexander Holler

BTW: I think this currently a bit a show stopper. One definitely wants 
to filter icmp and one definitely wants to save/restore rulesets. It is 
no problem for people which are writing their rulesets by hand, but 
those which are dynamically changing rules, likely are relying on the 
possibility to save and restore the whole ruleset (and being able to 
filter icmp).

[PATCH] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Context sensitive handling of "param-problem" and "redirect" is necessary
to allow usage of them as token or as string for icmp types.

Without this patch, e.g. the following fails:

nft add rule filter input icmp type redirect accept
nft add rule filter input icmpv6 type param-problem accept

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 src/parser_bison.y |  6 ++++--
 src/scanner.l      | 23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index b86381d..36a71d0 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -34,6 +34,8 @@
 
 #include "parser_bison.h"
 
+int icmp_flag;
+
 void parser_init(struct parser_state *state, struct list_head *msgs)
 {
 	memset(state, 0, sizeof(*state));
@@ -500,10 +502,10 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { expr_free($$); }	arp_hdr_expr
 %type <val>			arp_hdr_field
 %type <expr>			ip_hdr_expr	icmp_hdr_expr
-%destructor { expr_free($$); }	ip_hdr_expr	icmp_hdr_expr
+%destructor { expr_free($$); icmp_flag = 0; }	ip_hdr_expr	icmp_hdr_expr
 %type <val>			ip_hdr_field	icmp_hdr_field
 %type <expr>			ip6_hdr_expr    icmp6_hdr_expr
-%destructor { expr_free($$); }	ip6_hdr_expr	icmp6_hdr_expr
+%destructor { expr_free($$); icmp_flag = 0; }	ip6_hdr_expr	icmp6_hdr_expr
 %type <val>			ip6_hdr_field   icmp6_hdr_field
 %type <expr>			auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
 %destructor { expr_free($$); }	auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
diff --git a/src/scanner.l b/src/scanner.l
index 73c4f8b..3468276 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -100,6 +100,7 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 /* avoid warnings with -Wmissing-prototypes */
 extern int	yyget_column(yyscan_t);
 extern void	yyset_column(int, yyscan_t);
+extern int icmp_flag;
 
 %}
 
@@ -320,7 +321,14 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "snat"			{ return SNAT; }
 "dnat"			{ return DNAT; }
 "masquerade"		{ return MASQUERADE; }
-"redirect"		{ return REDIRECT; }
+"redirect"		{
+				if (icmp_flag == 4) {
+					yylval->string = xstrdup(yytext);
+					return STRING;
+				} else
+					return REDIRECT;
+			}
+
 "random"		{ return RANDOM; }
 "fully-random"		{ return FULLY_RANDOM; }
 "persistent"		{ return PERSISTENT; }
@@ -358,7 +366,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "protocol"		{ return PROTOCOL; }
 "checksum"		{ return CHECKSUM; }
 
-"icmp"			{ return ICMP; }
+"icmp"			{ icmp_flag = 4; return ICMP; }
 "code"			{ return CODE; }
 "sequence"		{ return SEQUENCE; }
 "gateway"		{ return GATEWAY; }
@@ -369,9 +377,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "flowlabel"		{ return FLOWLABEL; }
 "nexthdr"		{ return NEXTHDR; }
 "hoplimit"		{ return HOPLIMIT; }
+"icmpv6"		{ icmp_flag = 6; return ICMP6; }
+"param-problem"		{
+				if (icmp_flag == 6) {
+					yylval->string = xstrdup(yytext);
+					return STRING;
+				} else
+					return PPTR;
+			}
+
 
-"icmpv6"		{ return ICMP6; }
-"param-problem"		{ return PPTR; }
 "max-delay"		{ return MAXDELAY; }
 
 "ah"			{ return AH; }
-- 
2.1.0

Re: [PATCH] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 03.04.2015 um 19:50 schrieb Alexander Holler:
> Context sensitive handling of "param-problem" and "redirect" is necessary
> to allow usage of them as token or as string for icmp types.
>
> Without this patch, e.g. the following fails:
>
> nft add rule filter input icmp type redirect accept
> nft add rule filter input icmpv6 type param-problem accept
>
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---

Just in case of, I have not tested this extensively.

So please be careful with that patch and review it.

E.g. I'm not sure if I might have forgotten to set icmp_flag = 0 in 
another desctructor than those two I've added it too because I haven't 
tested rules which are using "redirect" or param-problem as token and 
not just as string to describe an icmp type as in the above two statements.

Regards,

Alexander Holler

Re: [PATCH] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 03.04.2015 um 20:06 schrieb Alexander Holler:
> Am 03.04.2015 um 19:50 schrieb Alexander Holler:
>> Context sensitive handling of "param-problem" and "redirect" is necessary
>> to allow usage of them as token or as string for icmp types.
>>
>> Without this patch, e.g. the following fails:
>>
>> nft add rule filter input icmp type redirect accept
>> nft add rule filter input icmpv6 type param-problem accept
>>
>> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
>> ---
>
> Just in case of, I have not tested this extensively.
>
> So please be careful with that patch and review it.
>
> E.g. I'm not sure if I might have forgotten to set icmp_flag = 0 in
> another desctructor than those two I've added it too because I haven't
> tested rules which are using "redirect" or param-problem as token and
> not just as string to describe an icmp type as in the above two statements.

Also I'm soliloquizing, here is an update.

Having had a second look at the parser, I think I indeed have forgotten 
a desctructor and this one should be changed too:

-%destructor { stmt_free($$); } reject_stmt reject_stmt_alloc
+%destructor { stmt_free($$); icmp_flag = 0; }  reject_stmt 
reject_stmt_alloc

I've now also written a small test-script which revealed an error in my 
kludges:

-- test-kludges.nft --
#!/sbin/nft -f

# small script to test the kludges (context sensitivity) for
# for "redirect" and "param-problem".

flush ruleset

table filter {
         chain input {
                 type filter hook input priority 0;
                 icmp type redirect accept
                 tcp dport 22223 reject with icmp type host-prohibited
         }
}
table ip6 filter {
         chain input {
                 type filter hook input priority 0;
                 icmpv6 type param-problem accept
                 tcp dport 22224 reject with icmpv6 type admin-prohibited
                 # THIS NOW FAILS:
                 #icmpv6 param-problem 2 drop
         }
}
table nat {
         chain prerouting {
                 type nat hook prerouting priority 0;
                 tcp dport 22222 redirect to 22
         }
         chain postrouting {
                 type nat hook postrouting priority 0;
         }
}
-- test-kludges.nft --


I'll already have an idea how to fix that and will post a second version 
of the patch when I've found the time to change and test it.


Regards,

Alexander Holler

[PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Context sensitive handling of "param-problem" and "redirect" is necessary
to allow usage of them as token or as string for icmp types.

Without this patch, e.g. the following fails:

nft add rule filter input icmp type redirect accept
nft add rule filter input icmpv6 type param-problem accept

And, worse, saving and restoring rules might fail. E.g. if the following
was used:

nft add rule filter input icmp type 5 accept

'nft list table filter' would show it using the type name "redirect" instead
of the the number, which means you could not use the output of 'nft list' to
restore the rules.

I've tested the patch using the following script:

---------------------
flush ruleset

table filter {
	chain input {
		type filter hook input priority 0;
		icmp type redirect accept
		tcp dport 22223 reject with icmp type host-prohibited
	}
}
table ip6 filter {
	chain input {
		type filter hook input priority 0;
		icmpv6 type param-problem accept
		tcp dport 22224 reject with icmpv6 type admin-prohibited
		icmpv6 param-problem 2 drop
	}
}
table nat {
	chain prerouting {
		type nat hook prerouting priority 0;
		tcp dport 22222 redirect to 22
	}
	chain postrouting {
		type nat hook postrouting priority 0;
	}
}
---------------------

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
 src/parser_bison.y |  8 +++++---
 src/scanner.l      | 30 ++++++++++++++++++++++++------
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index b86381d..af40195 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -34,6 +34,8 @@
 
 #include "parser_bison.h"
 
+int icmp_flag;
+
 void parser_init(struct parser_state *state, struct list_head *msgs)
 {
 	memset(state, 0, sizeof(*state));
@@ -445,7 +447,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { stmt_free($$); }	limit_stmt
 %type <val>			time_unit
 %type <stmt>			reject_stmt reject_stmt_alloc
-%destructor { stmt_free($$); }	reject_stmt reject_stmt_alloc
+%destructor { stmt_free($$); icmp_flag = 0; }	reject_stmt reject_stmt_alloc
 %type <stmt>			nat_stmt nat_stmt_alloc masq_stmt masq_stmt_alloc redir_stmt redir_stmt_alloc
 %destructor { stmt_free($$); }	nat_stmt nat_stmt_alloc masq_stmt masq_stmt_alloc redir_stmt redir_stmt_alloc
 %type <val>			nf_nat_flags nf_nat_flag
@@ -500,10 +502,10 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %destructor { expr_free($$); }	arp_hdr_expr
 %type <val>			arp_hdr_field
 %type <expr>			ip_hdr_expr	icmp_hdr_expr
-%destructor { expr_free($$); }	ip_hdr_expr	icmp_hdr_expr
+%destructor { expr_free($$); icmp_flag = 0; }	ip_hdr_expr	icmp_hdr_expr
 %type <val>			ip_hdr_field	icmp_hdr_field
 %type <expr>			ip6_hdr_expr    icmp6_hdr_expr
-%destructor { expr_free($$); }	ip6_hdr_expr	icmp6_hdr_expr
+%destructor { expr_free($$); icmp_flag = 0; }	ip6_hdr_expr	icmp6_hdr_expr
 %type <val>			ip6_hdr_field   icmp6_hdr_field
 %type <expr>			auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
 %destructor { expr_free($$); }	auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
diff --git a/src/scanner.l b/src/scanner.l
index 73c4f8b..3a058ad 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -100,6 +100,7 @@ static void reset_pos(struct parser_state *state, struct location *loc)
 /* avoid warnings with -Wmissing-prototypes */
 extern int	yyget_column(yyscan_t);
 extern void	yyset_column(int, yyscan_t);
+extern int icmp_flag;
 
 %}
 
@@ -320,7 +321,14 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "snat"			{ return SNAT; }
 "dnat"			{ return DNAT; }
 "masquerade"		{ return MASQUERADE; }
-"redirect"		{ return REDIRECT; }
+"redirect"		{
+				if (icmp_flag == 4) {
+					yylval->string = xstrdup(yytext);
+					return STRING;
+				} else
+					return REDIRECT;
+			}
+
 "random"		{ return RANDOM; }
 "fully-random"		{ return FULLY_RANDOM; }
 "persistent"		{ return PERSISTENT; }
@@ -334,8 +342,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "ether"			{ return ETHER; }
 "saddr"			{ return SADDR; }
 "daddr"			{ return DADDR; }
-"type"			{ return TYPE; }
-
+"type"			{
+				if (icmp_flag)
+					++icmp_flag;
+				return TYPE;
+			}
 "vlan"			{ return VLAN; }
 "id"			{ return ID; }
 "cfi"			{ return CFI; }
@@ -358,7 +369,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "protocol"		{ return PROTOCOL; }
 "checksum"		{ return CHECKSUM; }
 
-"icmp"			{ return ICMP; }
+"icmp"			{ icmp_flag = 3; return ICMP; }
 "code"			{ return CODE; }
 "sequence"		{ return SEQUENCE; }
 "gateway"		{ return GATEWAY; }
@@ -369,9 +380,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "flowlabel"		{ return FLOWLABEL; }
 "nexthdr"		{ return NEXTHDR; }
 "hoplimit"		{ return HOPLIMIT; }
+"icmpv6"		{ icmp_flag = 5; return ICMP6; }
+"param-problem"		{
+				if (icmp_flag == 6) {
+					yylval->string = xstrdup(yytext);
+					return STRING;
+				} else
+					return PPTR;
+			}
+
 
-"icmpv6"		{ return ICMP6; }
-"param-problem"		{ return PPTR; }
 "max-delay"		{ return MAXDELAY; }
 
 "ah"			{ return AH; }
-- 
2.1.0

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Pablo Neira Ayuso]

On Sat, Apr 04, 2015 at 01:13:06PM +0200, Alexander Holler wrote:
> Context sensitive handling of "param-problem" and "redirect" is necessary
> to allow usage of them as token or as string for icmp types.
[...]

I think we need some evaluation step at scanner level. This new
evaluation routine needs to understand the token semantics to set some
context information.

"redirect"		{ return scanner_evaluate(ctx, REDIRECT); }

We have to catch up more use cases such as sets and concatenations. I
started a patch here, a bit more generalized than this when you
reported this problem (we actually already knew about it).

@Patrick, any better idea?

> ---------------------
> 
> Signed-off-by: Alexander Holler <holler@ahsoftware.de>
> ---
>  src/parser_bison.y |  8 +++++---
>  src/scanner.l      | 30 ++++++++++++++++++++++++------
>  2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index b86381d..af40195 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -34,6 +34,8 @@
>  
>  #include "parser_bison.h"
>  
> +int icmp_flag;
> +
>  void parser_init(struct parser_state *state, struct list_head *msgs)
>  {
>  	memset(state, 0, sizeof(*state));
> @@ -445,7 +447,7 @@ static void location_update(struct location *loc, struct location *rhs, int n)
>  %destructor { stmt_free($$); }	limit_stmt
>  %type <val>			time_unit
>  %type <stmt>			reject_stmt reject_stmt_alloc
> -%destructor { stmt_free($$); }	reject_stmt reject_stmt_alloc
> +%destructor { stmt_free($$); icmp_flag = 0; }	reject_stmt reject_stmt_alloc
>  %type <stmt>			nat_stmt nat_stmt_alloc masq_stmt masq_stmt_alloc redir_stmt redir_stmt_alloc
>  %destructor { stmt_free($$); }	nat_stmt nat_stmt_alloc masq_stmt masq_stmt_alloc redir_stmt redir_stmt_alloc
>  %type <val>			nf_nat_flags nf_nat_flag
> @@ -500,10 +502,10 @@ static void location_update(struct location *loc, struct location *rhs, int n)
>  %destructor { expr_free($$); }	arp_hdr_expr
>  %type <val>			arp_hdr_field
>  %type <expr>			ip_hdr_expr	icmp_hdr_expr
> -%destructor { expr_free($$); }	ip_hdr_expr	icmp_hdr_expr
> +%destructor { expr_free($$); icmp_flag = 0; }	ip_hdr_expr	icmp_hdr_expr
>  %type <val>			ip_hdr_field	icmp_hdr_field
>  %type <expr>			ip6_hdr_expr    icmp6_hdr_expr
> -%destructor { expr_free($$); }	ip6_hdr_expr	icmp6_hdr_expr
> +%destructor { expr_free($$); icmp_flag = 0; }	ip6_hdr_expr	icmp6_hdr_expr
>  %type <val>			ip6_hdr_field   icmp6_hdr_field
>  %type <expr>			auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
>  %destructor { expr_free($$); }	auth_hdr_expr	esp_hdr_expr		comp_hdr_expr
> diff --git a/src/scanner.l b/src/scanner.l
> index 73c4f8b..3a058ad 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -100,6 +100,7 @@ static void reset_pos(struct parser_state *state, struct location *loc)
>  /* avoid warnings with -Wmissing-prototypes */
>  extern int	yyget_column(yyscan_t);
>  extern void	yyset_column(int, yyscan_t);
> +extern int icmp_flag;
>  
>  %}
>  
> @@ -320,7 +321,14 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "snat"			{ return SNAT; }
>  "dnat"			{ return DNAT; }
>  "masquerade"		{ return MASQUERADE; }
> -"redirect"		{ return REDIRECT; }
> +"redirect"		{
> +				if (icmp_flag == 4) {
> +					yylval->string = xstrdup(yytext);
> +					return STRING;
> +				} else
> +					return REDIRECT;
> +			}
> +
>  "random"		{ return RANDOM; }
>  "fully-random"		{ return FULLY_RANDOM; }
>  "persistent"		{ return PERSISTENT; }
> @@ -334,8 +342,11 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "ether"			{ return ETHER; }
>  "saddr"			{ return SADDR; }
>  "daddr"			{ return DADDR; }
> -"type"			{ return TYPE; }
> -
> +"type"			{
> +				if (icmp_flag)
> +					++icmp_flag;
> +				return TYPE;
> +			}
>  "vlan"			{ return VLAN; }
>  "id"			{ return ID; }
>  "cfi"			{ return CFI; }
> @@ -358,7 +369,7 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "protocol"		{ return PROTOCOL; }
>  "checksum"		{ return CHECKSUM; }
>  
> -"icmp"			{ return ICMP; }
> +"icmp"			{ icmp_flag = 3; return ICMP; }
>  "code"			{ return CODE; }
>  "sequence"		{ return SEQUENCE; }
>  "gateway"		{ return GATEWAY; }
> @@ -369,9 +380,16 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "flowlabel"		{ return FLOWLABEL; }
>  "nexthdr"		{ return NEXTHDR; }
>  "hoplimit"		{ return HOPLIMIT; }
> +"icmpv6"		{ icmp_flag = 5; return ICMP6; }
> +"param-problem"		{
> +				if (icmp_flag == 6) {
> +					yylval->string = xstrdup(yytext);
> +					return STRING;
> +				} else
> +					return PPTR;
> +			}
> +
>  
> -"icmpv6"		{ return ICMP6; }
> -"param-problem"		{ return PPTR; }
>  "max-delay"		{ return MAXDELAY; }
>  
>  "ah"			{ return AH; }
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 04.04.2015 um 13:55 schrieb Pablo Neira Ayuso:
> On Sat, Apr 04, 2015 at 01:13:06PM +0200, Alexander Holler wrote:
>> Context sensitive handling of "param-problem" and "redirect" is necessary
>> to allow usage of them as token or as string for icmp types.
> [...]
> 
> I think we need some evaluation step at scanner level. This new
> evaluation routine needs to understand the token semantics to set some
> context information.
> 
> "redirect"		{ return scanner_evaluate(ctx, REDIRECT); }
> 
> We have to catch up more use cases such as sets and concatenations. I
> started a patch here, a bit more generalized than this when you
> reported this problem (we actually already knew about it).
> 
> @Patrick, any better idea?

Hmm. Looks ambitious.

I've no idea if it's worse to spend the time to build a general solution
instead of doing it like I did. It looks like you want to build a state
machine inside that scanner_evaluate() which means you have to use it
for every token, if I've understood your idea correctly.

How many ambigious tokens do exist besides redirect and param-problem
for which I've now added a "mini state machine"?

Sorry, but I'm not actively following this project or the mailing lists,
and thus have no real overview over existing problems. I've just fixed a
problem I've encountered while switching some of my systems from
iptables to nftables.

Regards,

Alexander Holler

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

On 04.04, Alexander Holler wrote:
> Am 04.04.2015 um 13:55 schrieb Pablo Neira Ayuso:
> > On Sat, Apr 04, 2015 at 01:13:06PM +0200, Alexander Holler wrote:
> >> Context sensitive handling of "param-problem" and "redirect" is necessary
> >> to allow usage of them as token or as string for icmp types.
> > [...]
> > 
> > I think we need some evaluation step at scanner level. This new
> > evaluation routine needs to understand the token semantics to set some
> > context information.
> > 
> > "redirect"		{ return scanner_evaluate(ctx, REDIRECT); }
> > 
> > We have to catch up more use cases such as sets and concatenations. I
> > started a patch here, a bit more generalized than this when you
> > reported this problem (we actually already knew about it).
> > 
> > @Patrick, any better idea?
> 
> Hmm. Looks ambitious.
> 
> I've no idea if it's worse to spend the time to build a general solution
> instead of doing it like I did. It looks like you want to build a state
> machine inside that scanner_evaluate() which means you have to use it
> for every token, if I've understood your idea correctly.
> 
> How many ambigious tokens do exist besides redirect and param-problem
> for which I've now added a "mini state machine"?

These cases keep coming up and we actually can't even know all cases
since many expressions use externally defined constants, f.i. services,
routing marks, realms etc, interface names and many more. We really
need a generic solution, at least in the mid term.

> Sorry, but I'm not actively following this project or the mailing lists,
> and thus have no real overview over existing problems. I've just fixed a
> problem I've encountered while switching some of my systems from
> iptables to nftables.

I'm not opposed to a short term fix until we have something generic.
However we used a different approach so far (for different cases though)
and I'd like to at least check whether something similar will work
in this case.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

On 04.04, Pablo Neira Ayuso wrote:
> On Sat, Apr 04, 2015 at 01:13:06PM +0200, Alexander Holler wrote:
> > Context sensitive handling of "param-problem" and "redirect" is necessary
> > to allow usage of them as token or as string for icmp types.
> [...]
> 
> I think we need some evaluation step at scanner level. This new
> evaluation routine needs to understand the token semantics to set some
> context information.
> 
> "redirect"		{ return scanner_evaluate(ctx, REDIRECT); }
> 
> We have to catch up more use cases such as sets and concatenations. I
> started a patch here, a bit more generalized than this when you
> reported this problem (we actually already knew about it).
> 
> @Patrick, any better idea?

This won't work because the grammar currently allows both cases.

The proper solution IMO is to change the grammar so we know where such
keywords are keywords and where they are constants.

Basically this involves splitting the expression types into lhs (non-const)
and rhs (const) parts. Keywords on the RHS side can be caught using an
error statement and deferred to resolution during runtime.

The second benefit is that we can then use the grammar for tab completion.

I have patches for this, but as I'm currently trying to get the remaining
set and concat patches submitted, I won't be able to work on this until
after the merge window closes.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On 05.04, Patrick McHardy wrote:
> On 04.04, Pablo Neira Ayuso wrote:
> > On Sat, Apr 04, 2015 at 01:13:06PM +0200, Alexander Holler wrote:
> > > Context sensitive handling of "param-problem" and "redirect" is necessary
> > > to allow usage of them as token or as string for icmp types.
> > [...]
> > 
> > I think we need some evaluation step at scanner level. This new
> > evaluation routine needs to understand the token semantics to set some
> > context information.
> > 
> > "redirect"		{ return scanner_evaluate(ctx, REDIRECT); }
> > 
> > We have to catch up more use cases such as sets and concatenations. I
> > started a patch here, a bit more generalized than this when you
> > reported this problem (we actually already knew about it).
> > 
> > @Patrick, any better idea?
> 
> This won't work because the grammar currently allows both cases.
> 
> The proper solution IMO is to change the grammar so we know where such
> keywords are keywords and where they are constants.
> 
> Basically this involves splitting the expression types into lhs (non-const)
> and rhs (const) parts. Keywords on the RHS side can be caught using an
> error statement and deferred to resolution during runtime.

Actually, it even seems to work without doing the splitting. This patch
shows the basic idea. We add a error token to symbol_expr, convert the
erroneous keyword to a symbolic expression and push it to the evaluation
step. Without the split to LHS/RHS it can't handle cases like "TCP",
but it does handle all keywords that are not the first one of an expression.

The redirect case seems to be working fine:

<cmdline>:1:15-23: Evaluate
filter output icmp type redirect
              ^^^^^^^^^
ip protocol

<cmdline>:1:15-23: Evaluate
filter output icmp type redirect
              ^^^^^^^^^
icmp

<cmdline>:1:25-32: Evaluate
filter output icmp type redirect
                        ^^^^^^^^
$redirect

<cmdline>:1:25-32: Evaluate
filter output icmp type redirect
                        ^^^^^^^^
redirect

ip filter output 
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000001 ]
  [ payload load 1b @ transport header + 0 => reg 1 ]
  [ cmp eq reg 1 0x00000005 ]

This needs a lot of testing though since it has the potential to break
things quite badly. Since I'm busy, maybe someone else wants to start by
running the testsuite with this patch applied.

[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 758 bytes --]

diff --git a/src/parser_bison.y b/src/parser_bison.y
index b86381d..8d39c67 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1583,6 +1583,30 @@ symbol_expr		:	string
 						       $2);
 				xfree($2);
 			}
+			|	error
+			{
+				struct error_record *erec;
+				char *tmp;
+
+				if (yytoken != TOKEN_EOF) {
+					tmp = xstrdup(yytname[yytoken] + 1);
+					tmp[strlen(tmp) - 1] = '\0';
+					$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
+							       current_scope(state),
+							       tmp);
+					xfree(tmp);
+
+					erec = list_entry(state->msgs->prev,
+							  struct error_record, list);
+					list_del(&erec->list);
+					xfree(erec);
+
+					yyclearin;
+					yyerrok;
+				} else {
+					YYABORT;
+				}
+			}
 			;
 
 integer_expr		:	NUM

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
> On 05.04, Patrick McHardy wrote:

>> Basically this involves splitting the expression types into lhs (non-const)
>> and rhs (const) parts. Keywords on the RHS side can be caught using an
>> error statement and deferred to resolution during runtime.

Sounds like trial and error. ;)

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

On 05.04, Alexander Holler wrote:
> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
> > On 05.04, Patrick McHardy wrote:
> 
> >> Basically this involves splitting the expression types into lhs (non-const)
> >> and rhs (const) parts. Keywords on the RHS side can be caught using an
> >> error statement and deferred to resolution during runtime.
> 
> Sounds like trial and error. ;)

The approach is, the patch isn't, it changes the grammar to have
these kinds of errors in a defined state. The patch I sent
however is, but I'm quite sure i understand the implications.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
> On 05.04, Alexander Holler wrote:
>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
>>> On 05.04, Patrick McHardy wrote:
>>
>>>> Basically this involves splitting the expression types into lhs (non-const)
>>>> and rhs (const) parts. Keywords on the RHS side can be caught using an
>>>> error statement and deferred to resolution during runtime.
>>
>> Sounds like trial and error. ;)
>
> The approach is, the patch isn't, it changes the grammar to have
> these kinds of errors in a defined state. The patch I sent
> however is, but I'm quite sure i understand the implications.

Just to mention it, there is still the possibility to define and use 
keywords for all the icmp type names.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 06.04.2015 um 10:44 schrieb Alexander Holler:
> Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
>> On 05.04, Alexander Holler wrote:
>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
>>>> On 05.04, Patrick McHardy wrote:
>>>
>>>>> Basically this involves splitting the expression types into lhs
>>>>> (non-const)
>>>>> and rhs (const) parts. Keywords on the RHS side can be caught using an
>>>>> error statement and deferred to resolution during runtime.
>>>
>>> Sounds like trial and error. ;)
>>
>> The approach is, the patch isn't, it changes the grammar to have
>> these kinds of errors in a defined state. The patch I sent
>> however is, but I'm quite sure i understand the implications.
>
> Just to mention it, there is still the possibility to define and use
> keywords for all the icmp type names.

Preferable with names as used in icmp.h and icmpv6.h. As these are 
defines in C-headers, there is very high probability that these names 
are unique, even across a large number of different sets of type or 
other names.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 06.04.2015 um 11:01 schrieb Alexander Holler:
> Am 06.04.2015 um 10:44 schrieb Alexander Holler:
>> Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
>>> On 05.04, Alexander Holler wrote:
>>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
>>>>> On 05.04, Patrick McHardy wrote:
>>>>
>>>>>> Basically this involves splitting the expression types into lhs
>>>>>> (non-const)
>>>>>> and rhs (const) parts. Keywords on the RHS side can be caught
>>>>>> using an
>>>>>> error statement and deferred to resolution during runtime.
>>>>
>>>> Sounds like trial and error. ;)
>>>
>>> The approach is, the patch isn't, it changes the grammar to have
>>> these kinds of errors in a defined state. The patch I sent
>>> however is, but I'm quite sure i understand the implications.
>>
>> Just to mention it, there is still the possibility to define and use
>> keywords for all the icmp type names.
>
> Preferable with names as used in icmp.h and icmpv6.h. As these are
> defines in C-headers, there is very high probability that these names
> are unique, even across a large number of different sets of type or
> other names.

That would also remove the need to look up what name nft uses if would 
be clear that the names are the same as defined in c-headers.

E.g. the ICMPv6 parameter-problem is good example. In the linux headers 
it is called ICMPV6_PARAMPROB, nft named it param-problem and in 
documentations it is often named as parameter-problem.

So if nft would use icmpv6_paramprob, the documentation could just refer 
to the c-headers.

Regards,

Alexander Holler

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

On 06.04, Alexander Holler wrote:
> Am 06.04.2015 um 11:01 schrieb Alexander Holler:
> >Am 06.04.2015 um 10:44 schrieb Alexander Holler:
> >>Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
> >>>On 05.04, Alexander Holler wrote:
> >>>>Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
> >>>>>On 05.04, Patrick McHardy wrote:
> >>>>
> >>>>>>Basically this involves splitting the expression types into lhs
> >>>>>>(non-const)
> >>>>>>and rhs (const) parts. Keywords on the RHS side can be caught
> >>>>>>using an
> >>>>>>error statement and deferred to resolution during runtime.
> >>>>
> >>>>Sounds like trial and error. ;)
> >>>
> >>>The approach is, the patch isn't, it changes the grammar to have
> >>>these kinds of errors in a defined state. The patch I sent
> >>>however is, but I'm quite sure i understand the implications.
> >>
> >>Just to mention it, there is still the possibility to define and use
> >>keywords for all the icmp type names.
> >
> >Preferable with names as used in icmp.h and icmpv6.h. As these are
> >defines in C-headers, there is very high probability that these names
> >are unique, even across a large number of different sets of type or
> >other names.
> 
> That would also remove the need to look up what name nft uses if would be
> clear that the names are the same as defined in c-headers.
> 
> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it
> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations
> it is often named as parameter-problem.
> 
> So if nft would use icmpv6_paramprob, the documentation could just refer to
> the c-headers.

No, an icmpv6_ prefix is redundant and I don't see any benefit in doing
that. "Documentations", whatever that is, don't matter, what matters is
our documentation. And whether we point people to that or some header
file really doesn't matter, except that we don't expect people to read
header files to use nft. Its a tool for admins, not programmers.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Alexander Holler]

Am 06.04.2015 um 13:25 schrieb Patrick McHardy:
> On 06.04, Alexander Holler wrote:
>> Am 06.04.2015 um 11:01 schrieb Alexander Holler:
>>> Am 06.04.2015 um 10:44 schrieb Alexander Holler:
>>>> Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
>>>>> On 05.04, Alexander Holler wrote:
>>>>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
>>>>>>> On 05.04, Patrick McHardy wrote:
>>>>>>
>>>>>>>> Basically this involves splitting the expression types into lhs
>>>>>>>> (non-const)
>>>>>>>> and rhs (const) parts. Keywords on the RHS side can be caught
>>>>>>>> using an
>>>>>>>> error statement and deferred to resolution during runtime.
>>>>>>
>>>>>> Sounds like trial and error. ;)
>>>>>
>>>>> The approach is, the patch isn't, it changes the grammar to have
>>>>> these kinds of errors in a defined state. The patch I sent
>>>>> however is, but I'm quite sure i understand the implications.
>>>>
>>>> Just to mention it, there is still the possibility to define and use
>>>> keywords for all the icmp type names.
>>>
>>> Preferable with names as used in icmp.h and icmpv6.h. As these are
>>> defines in C-headers, there is very high probability that these names
>>> are unique, even across a large number of different sets of type or
>>> other names.
>>
>> That would also remove the need to look up what name nft uses if would be
>> clear that the names are the same as defined in c-headers.
>>
>> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it
>> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations
>> it is often named as parameter-problem.
>>
>> So if nft would use icmpv6_paramprob, the documentation could just refer to
>> the c-headers.
> 
> No, an icmpv6_ prefix is redundant and I don't see any benefit in doing
> that. "Documentations", whatever that is, don't matter, what matters is
> our documentation. And whether we point people to that or some header
> file really doesn't matter, except that we don't expect people to read
> header files to use nft. Its a tool for admins, not programmers.

Obviously it isn't redundant, otherwise I wouldn't had to write a patch
for a problem which now exists since quiet some time (can't remember
when I've first stumbled over it, maybe half a year or even a year).

Anyway, thanks for fixing it, regardless if you apply my patch or
something else.

Regards,

Alexander Holler

nft parser and names for constants (was [PATCH v2] parser: add kludges for "param-problem" and "redirect")

[Alexander Holler]

Am 06.04.2015 um 13:25 schrieb Patrick McHardy:
> On 06.04, Alexander Holler wrote:
>> Am 06.04.2015 um 11:01 schrieb Alexander Holler:
>>> Am 06.04.2015 um 10:44 schrieb Alexander Holler:
>>>> Am 06.04.2015 um 03:51 schrieb Patrick McHardy:
>>>>> On 05.04, Alexander Holler wrote:
>>>>>> Am 05.04.2015 um 14:11 schrieb Patrick McHardy:
>>>>>>> On 05.04, Patrick McHardy wrote:
>>>>>>
>>>>>>>> Basically this involves splitting the expression types into lhs
>>>>>>>> (non-const)
>>>>>>>> and rhs (const) parts. Keywords on the RHS side can be caught
>>>>>>>> using an
>>>>>>>> error statement and deferred to resolution during runtime.
>>>>>>
>>>>>> Sounds like trial and error. ;)
>>>>>
>>>>> The approach is, the patch isn't, it changes the grammar to have
>>>>> these kinds of errors in a defined state. The patch I sent
>>>>> however is, but I'm quite sure i understand the implications.
>>>>
>>>> Just to mention it, there is still the possibility to define and use
>>>> keywords for all the icmp type names.
>>>
>>> Preferable with names as used in icmp.h and icmpv6.h. As these are
>>> defines in C-headers, there is very high probability that these names
>>> are unique, even across a large number of different sets of type or
>>> other names.
>>
>> That would also remove the need to look up what name nft uses if would be
>> clear that the names are the same as defined in c-headers.
>>
>> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it
>> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations
>> it is often named as parameter-problem.
>>
>> So if nft would use icmpv6_paramprob, the documentation could just refer to
>> the c-headers.
>
> No, an icmpv6_ prefix is redundant and I don't see any benefit in doing
> that. "Documentations", whatever that is, don't matter, what matters is
> our documentation. And whether we point people to that or some header
> file really doesn't matter, except that we don't expect people to read
> header files to use nft. Its a tool for admins, not programmers.

Even some admins can code.


Because nft is just at 0.4 and not widely used (or usable), I decided 
it's worse to write another follow up in order to try to fix things 
before they can't be fixed anymore.


1. I don't think that a brutforce-approach like "if error try something 
else" is the right way to fix a problem in the parser.

2. "icmpv6_paramprob" (or even something like 
"icmpv6_parameter-problem") doesn't have something redundant. It's a 
name for constant and e.g. "icmp_redirect" is a much more descriptive 
name than just "redirect" because "icmp_redirect" describes the context 
too and thus the name is much more verbose and readable (besides that it 
would fix the problem the parser currently has).

3. I don't see why admins have to use another set of names for constants 
than developers. Inventing a new set of names for a list of constants 
for which there already exist a very widely used set of names just leads 
to more confusion. And if it's ok to invent new names, why does nft use 
"param-problem" and not "parameter-problem"? Of course, I would suggest 
to use the existing name icmp_parameterprob (like it's used in every 
c/c++-source).

Alexander Holler

Re: nft parser and names for constants (was [PATCH v2] parser: add kludges for "param-problem" and "redirect")

[Patrick McHardy]

On 09.04, Alexander Holler wrote:
> >>E.g. the ICMPv6 parameter-problem is good example. In the linux headers it
> >>is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations
> >>it is often named as parameter-problem.
> >>
> >>So if nft would use icmpv6_paramprob, the documentation could just refer to
> >>the c-headers.
> >
> >No, an icmpv6_ prefix is redundant and I don't see any benefit in doing
> >that. "Documentations", whatever that is, don't matter, what matters is
> >our documentation. And whether we point people to that or some header
> >file really doesn't matter, except that we don't expect people to read
> >header files to use nft. Its a tool for admins, not programmers.
> 
> Even some admins can code.
> 
> Because nft is just at 0.4 and not widely used (or usable), I decided it's
> worse to write another follow up in order to try to fix things before they
> can't be fixed anymore.
> 
> 1. I don't think that a brutforce-approach like "if error try something
> else" is the right way to fix a problem in the parser.

Its perfectly fine in this case since the error is only caught in spots
where we expect symbolic constants. So basically its treating the next
keyword as symbolic constant no matter what.

> 2. "icmpv6_paramprob" (or even something like "icmpv6_parameter-problem")
> doesn't have something redundant. It's a name for constant and e.g.
> "icmp_redirect" is a much more descriptive name than just "redirect" because
> "icmp_redirect" describes the context too and thus the name is much more
> verbose and readable (besides that it would fix the problem the parser
> currently has).

It is redundant since it can only occur in the context of ICMP expressions.
icmp type icmp_redirect is obviously redundant.

> 3. I don't see why admins have to use another set of names for constants
> than developers. Inventing a new set of names for a list of constants for
> which there already exist a very widely used set of names just leads to more
> confusion. And if it's ok to invent new names, why does nft use
> "param-problem" and not "parameter-problem"? Of course, I would suggest to
> use the existing name icmp_parameterprob (like it's used in every
> c/c++-source).

In case of ICMP we use the same names that iptables used, so this actually
spares admins from getting used to new constants. We're not going to use
source code identifiers, there's no benefit at all, especially if you
consider that Linux headers use different identifiers than the BSD headers.

But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for
consistency with both ICMP and ip6tables.

Re: nft parser and names for constants (was [PATCH v2] parser: add kludges for "param-problem" and "redirect")

[Alexander Holler]

Am 09.04.2015 um 13:07 schrieb Patrick McHardy:
> On 09.04, Alexander Holler wrote:
>>>> E.g. the ICMPv6 parameter-problem is good example. In the linux headers it
>>>> is called ICMPV6_PARAMPROB, nft named it param-problem and in documentations
>>>> it is often named as parameter-problem.
>>>>
>>>> So if nft would use icmpv6_paramprob, the documentation could just refer to
>>>> the c-headers.
>>>
>>> No, an icmpv6_ prefix is redundant and I don't see any benefit in doing
>>> that. "Documentations", whatever that is, don't matter, what matters is
>>> our documentation. And whether we point people to that or some header
>>> file really doesn't matter, except that we don't expect people to read
>>> header files to use nft. Its a tool for admins, not programmers.
>>
>> Even some admins can code.
>>
>> Because nft is just at 0.4 and not widely used (or usable), I decided it's
>> worse to write another follow up in order to try to fix things before they
>> can't be fixed anymore.
>>
>> 1. I don't think that a brutforce-approach like "if error try something
>> else" is the right way to fix a problem in the parser.
>
> Its perfectly fine in this case since the error is only caught in spots
> where we expect symbolic constants. So basically its treating the next
> keyword as symbolic constant no matter what.

I wonder how the error routine in that patch knowns that a symbolic 
constant is expected. Where does it get that information from? And if 
so, why is in such a case error called anyways? And how are problems in 
the grammar identified in which a token or string might match? Of 
course, my bison knowledge got very rusty as I haven't fiddled with it 
for a long time, but I still think it's brute-force or trial-and-error 
approach.

>> 2. "icmpv6_paramprob" (or even something like "icmpv6_parameter-problem")
>> doesn't have something redundant. It's a name for constant and e.g.
>> "icmp_redirect" is a much more descriptive name than just "redirect" because
>> "icmp_redirect" describes the context too and thus the name is much more
>> verbose and readable (besides that it would fix the problem the parser
>> currently has).
>
> It is redundant since it can only occur in the context of ICMP expressions.
> icmp type icmp_redirect is obviously redundant.

It's just a name as such it can't be redundant if you want to allow 
names. Of course, there might be people which are unaware that "icmp_" 
in "icmp_redirect" is just part of the name for a constant, but those 
people shouldn't write firewall rules anyway.


>> 3. I don't see why admins have to use another set of names for constants
>> than developers. Inventing a new set of names for a list of constants for
>> which there already exist a very widely used set of names just leads to more
>> confusion. And if it's ok to invent new names, why does nft use
>> "param-problem" and not "parameter-problem"? Of course, I would suggest to
>> use the existing name icmp_parameterprob (like it's used in every
>> c/c++-source).
>
> In case of ICMP we use the same names that iptables used, so this actually
> spares admins from getting used to new constants. We're not going to use
> source code identifiers, there's no benefit at all, especially if you
> consider that Linux headers use different identifiers than the BSD headers.

nft isn't in use on BSD and if you think taking BSD out of a corner 
makes sense, I wonder how compatible the names, nft uses, are, with what 
is used by ipf or one of the other BSD firewall packages. As the answer 
is the names are incompatible, arguing with BSD is nonsense here.

>
> But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for
> consistency with both ICMP and ip6tables.

Anyway, I've tried it a second time. Can't do more. So I will now 
entering the popcorn-state.

Alexander Holler

Re: nft parser and names for constants (was [PATCH v2] parser: add kludges for "param-problem" and "redirect")

[Patrick McHardy]

On 09.04, Alexander Holler wrote:
> Am 09.04.2015 um 13:07 schrieb Patrick McHardy:
> >>3. I don't see why admins have to use another set of names for constants
> >>than developers. Inventing a new set of names for a list of constants for
> >>which there already exist a very widely used set of names just leads to more
> >>confusion. And if it's ok to invent new names, why does nft use
> >>"param-problem" and not "parameter-problem"? Of course, I would suggest to
> >>use the existing name icmp_parameterprob (like it's used in every
> >>c/c++-source).
> >
> >In case of ICMP we use the same names that iptables used, so this actually
> >spares admins from getting used to new constants. We're not going to use
> >source code identifiers, there's no benefit at all, especially if you
> >consider that Linux headers use different identifiers than the BSD headers.
> 
> nft isn't in use on BSD and if you think taking BSD out of a corner makes
> sense, I wonder how compatible the names, nft uses, are, with what is used
> by ipf or one of the other BSD firewall packages. As the answer is the names
> are incompatible, arguing with BSD is nonsense here.

This is starting to annoy me. If you suggest to use names from headers, at
least do your homework. The BSD headers is what most of userspace uses, and
this is where ICMP_PARAMPROB originates. Linux uses ICMP_PARAMETERPROB.

> >But I agree, ICMPv6 shouldn't use param-problem but parameter-problem for
> >consistency with both ICMP and ip6tables.
> 
> Anyway, I've tried it a second time. Can't do more. So I will now entering
> the popcorn-state.

The decision has been made a long time ago. You're wasting both our time.

Re: nft parser and names for constants (was [PATCH v2] parser: add kludges for "param-problem" and "redirect")

[Alexander Holler]

Am 09.04.2015 um 21:15 schrieb Patrick McHardy:
> On 09.04, Alexander Holler wrote:
>> Am 09.04.2015 um 13:07 schrieb Patrick McHardy:
>>>> 3. I don't see why admins have to use another set of names for constants
>>>> than developers. Inventing a new set of names for a list of constants for
>>>> which there already exist a very widely used set of names just leads to more
>>>> confusion. And if it's ok to invent new names, why does nft use
>>>> "param-problem" and not "parameter-problem"? Of course, I would suggest to
>>>> use the existing name icmp_parameterprob (like it's used in every
>>>> c/c++-source).
>>>
>>> In case of ICMP we use the same names that iptables used, so this actually
>>> spares admins from getting used to new constants. We're not going to use
>>> source code identifiers, there's no benefit at all, especially if you
>>> consider that Linux headers use different identifiers than the BSD headers.
>>
>> nft isn't in use on BSD and if you think taking BSD out of a corner makes
>> sense, I wonder how compatible the names, nft uses, are, with what is used
>> by ipf or one of the other BSD firewall packages. As the answer is the names
>> are incompatible, arguing with BSD is nonsense here.
>
> This is starting to annoy me. If you suggest to use names from headers, at
> least do your homework. The BSD headers is what most of userspace uses, and
> this is where ICMP_PARAMPROB originates. Linux uses ICMP_PARAMETERPROB.

I aggree that this discussion, which never really has begun, finally 
ended in the mud. Feel free to throw out more accusations as I won't try 
to discuss with you anymore.

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Arturo Borrero Gonzalez]

On 5 April 2015 at 14:11, Patrick McHardy <kaber@trash.net> wrote:
>
> This needs a lot of testing though since it has the potential to break
> things quite badly. Since I'm busy, maybe someone else wants to start by
> running the testsuite with this patch applied.

I was curious and ran the testsuite with your patch. It's OK.

best regards.

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] parser: add kludges for "param-problem" and "redirect"

[Patrick McHardy]

On 06.04, Arturo Borrero Gonzalez wrote:
> On 5 April 2015 at 14:11, Patrick McHardy <kaber@trash.net> wrote:
> >
> > This needs a lot of testing though since it has the potential to break
> > things quite badly. Since I'm busy, maybe someone else wants to start by
> > running the testsuite with this patch applied.
> 
> I was curious and ran the testsuite with your patch. It's OK.

Thanks Arturo!