[nft PATCH 1/2] erec: Make __stmt_binary_error a little more versatile

[nft PATCH 1/2] erec: Make __stmt_binary_error a little more versatile

[Phil Sutter]

In order to allow for printing (at least) also warnings, rename
__stmt_binary_error to __stmt_binary_msg and pass the severity as first
parameter. Then create __stmt_binary_error macro to reestablish the old
signature.

Finally, add similar macros for printing warnings. And since these are
supposed to be non-fatal, make __stmt_binary_msg return 0 if a severity
below EREC_ERROR was given. This allows to still use it in return
statements.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/erec.h | 16 ++++++++++++----
 src/erec.c     | 13 +++++++------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/erec.h b/include/erec.h
index 36e0efa4fc1d9..0babfc0ff488c 100644
--- a/include/erec.h
+++ b/include/erec.h
@@ -63,11 +63,19 @@ extern void erec_print_list(FILE *f, struct list_head *list);
 
 struct eval_ctx;
 
-extern int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
-						 const struct location *l1,
-						 const struct location *l2,
-						 const char *fmt, ...);
+extern int __fmtstring(5, 6) __stmt_binary_msg(enum error_record_types type,
+					       struct eval_ctx *ctx,
+					       const struct location *l1,
+					       const struct location *l2,
+					       const char *fmt, ...);
 
+#define __stmt_binary_warning(ctx, l1, l2, fmt, args...) \
+	__stmt_binary_msg(EREC_WARNING, ctx, l1, l2, fmt, ## args)
+#define stmt_warning(ctx, s1, fmt, args...) \
+	__stmt_binary_warning(ctx, &(s1)->location, NULL, fmt, ## args)
+
+#define __stmt_binary_error(ctx, l1, l2, fmt, args...) \
+	__stmt_binary_msg(EREC_ERROR, ctx, l1, l2, fmt, ## args)
 #define stmt_error(ctx, s1, fmt, args...) \
 	__stmt_binary_error(ctx, &(s1)->location, NULL, fmt, ## args)
 #define stmt_binary_error(ctx, s1, s2, fmt, args...) \
diff --git a/src/erec.c b/src/erec.c
index eacdd97a21cbc..7b9c2420d416c 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -208,19 +208,20 @@ void erec_print_list(FILE *f, struct list_head *list)
 	}
 }
 
-int __fmtstring(4, 5) __stmt_binary_error(struct eval_ctx *ctx,
-					  const struct location *l1,
-					  const struct location *l2,
-					  const char *fmt, ...)
+int __fmtstring(5, 6) __stmt_binary_msg(enum error_record_types type,
+					struct eval_ctx *ctx,
+					const struct location *l1,
+					const struct location *l2,
+					const char *fmt, ...)
 {
 	struct error_record *erec;
 	va_list ap;
 
 	va_start(ap, fmt);
-	erec = erec_vcreate(EREC_ERROR, l1, fmt, ap);
+	erec = erec_vcreate(type, l1, fmt, ap);
 	if (l2 != NULL)
 		erec_add_location(erec, l2);
 	va_end(ap);
 	erec_queue(erec, ctx->msgs);
-	return -1;
+	return type >= EREC_ERROR ? -1 : 0;
 }
-- 
2.11.0

[nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Phil Sutter]

As reported in netfilter bz#1105, masquerading won't work if there isn't
at least an empty base chain hooked into prerouting. In order to raise
awareness of this problem at the user, complain if a masquerading
statement is added and the table does not contain an appropriate
prerouting chain already.

To not break user scripts which add the required chain at a later point,
accept the command anyway.

A better solution would be to create the required chain as a dependency
and drop it again on return path or if the user adds his own one later,
though I doubt the extra effort is feasible here.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 49c5953ae1687..a89ada19298a5 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2379,6 +2379,8 @@ static int stmt_evaluate_nat(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_masq(struct eval_ctx *ctx, struct stmt *stmt)
 {
+	struct table *table;
+	struct chain *chain;
 	int err;
 
 	err = nat_evaluate_family(ctx, stmt);
@@ -2392,7 +2394,23 @@ static int stmt_evaluate_masq(struct eval_ctx *ctx, struct stmt *stmt)
 	}
 
 	stmt->flags |= STMT_F_TERMINAL;
-	return 0;
+
+	err = cache_update(CMD_INVALID, ctx->msgs);
+	if (err < 0)
+		return err;
+
+	table = table_lookup_global(ctx);
+	if (!table)
+		return stmt_error(ctx, stmt, "referenced table not found");
+
+	list_for_each_entry(chain, &table->chains, list) {
+		if (!strcmp(chain->type, "nat") &&
+		    chain->hooknum == NF_INET_PRE_ROUTING)
+			return 0;
+	}
+
+	return stmt_warning(ctx, stmt,
+			    "this requires at least an empty prerouting chain");
 }
 
 static int stmt_evaluate_redir(struct eval_ctx *ctx, struct stmt *stmt)
-- 
2.11.0

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Arturo Borrero Gonzalez]

On 27 April 2017 at 15:24, Phil Sutter <phil@nwl.cc> wrote:
> As reported in netfilter bz#1105, masquerading won't work if there isn't
> at least an empty base chain hooked into prerouting. In order to raise
> awareness of this problem at the user, complain if a masquerading
> statement is added and the table does not contain an appropriate
> prerouting chain already.
>
> To not break user scripts which add the required chain at a later point,
> accept the command anyway.
>
> A better solution would be to create the required chain as a dependency
> and drop it again on return path or if the user adds his own one later,
> though I doubt the extra effort is feasible here.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/evaluate.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>

This warning will be printed even in rulesets loaded with '-f'
which first creates the masq rule an then the other chain.

I think is just a matter of documenting *everywhere* that this is the
expected behaviour, not a bug.

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Phil Sutter]

On Fri, Apr 28, 2017 at 09:01:44AM +0200, Arturo Borrero Gonzalez wrote:
> On 27 April 2017 at 15:24, Phil Sutter <phil@nwl.cc> wrote:
> > As reported in netfilter bz#1105, masquerading won't work if there isn't
> > at least an empty base chain hooked into prerouting. In order to raise
> > awareness of this problem at the user, complain if a masquerading
> > statement is added and the table does not contain an appropriate
> > prerouting chain already.
> >
> > To not break user scripts which add the required chain at a later point,
> > accept the command anyway.
> >
> > A better solution would be to create the required chain as a dependency
> > and drop it again on return path or if the user adds his own one later,
> > though I doubt the extra effort is feasible here.
> >
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/evaluate.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> 
> This warning will be printed even in rulesets loaded with '-f'
> which first creates the masq rule an then the other chain.

Hmm. I tested it with the following config and it works fine:

| table ip nat {
| 	chain post {
| 		type nat hook postrouting priority 0; policy accept;
| 		oifname "veth2" masquerade
| 	}
| 
| 	chain pre {
| 		type nat hook prerouting priority 0; policy accept;
| 	}
| }

OK, with a config consisting of several 'add' commands, it indeed warns.

> I think is just a matter of documenting *everywhere* that this is the
> expected behaviour, not a bug.

Yeah, I should indeed have done that first, also because masquerade
statement is not documented at all yet.

Thanks, Phil

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Arturo Borrero Gonzalez]

On 28 April 2017 at 10:05, Phil Sutter <phil@nwl.cc> wrote:
>>
>> This warning will be printed even in rulesets loaded with '-f'
>> which first creates the masq rule an then the other chain.
>
> Hmm. I tested it with the following config and it works fine:
>
> | table ip nat {
> |       chain post {
> |               type nat hook postrouting priority 0; policy accept;
> |               oifname "veth2" masquerade
> |       }
> |
> |       chain pre {
> |               type nat hook prerouting priority 0; policy accept;
> |       }
> | }
>
> OK, with a config consisting of several 'add' commands, it indeed warns.
>
>> I think is just a matter of documenting *everywhere* that this is the
>> expected behaviour, not a bug.
>
> Yeah, I should indeed have done that first, also because masquerade
> statement is not documented at all yet.
>

The best current documentation is this:

https://wiki.nftables.org/wiki-nftables/index.php/Performing_Network_Address_Translation_(NAT)

It can be improved, though

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Phil Sutter]

On Fri, Apr 28, 2017 at 10:11:51AM +0200, Arturo Borrero Gonzalez wrote:
> On 28 April 2017 at 10:05, Phil Sutter <phil@nwl.cc> wrote:
> >>
> >> This warning will be printed even in rulesets loaded with '-f'
> >> which first creates the masq rule an then the other chain.
> >
> > Hmm. I tested it with the following config and it works fine:
> >
> > | table ip nat {
> > |       chain post {
> > |               type nat hook postrouting priority 0; policy accept;
> > |               oifname "veth2" masquerade
> > |       }
> > |
> > |       chain pre {
> > |               type nat hook prerouting priority 0; policy accept;
> > |       }
> > | }
> >
> > OK, with a config consisting of several 'add' commands, it indeed warns.
> >
> >> I think is just a matter of documenting *everywhere* that this is the
> >> expected behaviour, not a bug.
> >
> > Yeah, I should indeed have done that first, also because masquerade
> > statement is not documented at all yet.
> >
> 
> The best current documentation is this:
> 
> https://wiki.nftables.org/wiki-nftables/index.php/Performing_Network_Address_Translation_(NAT)

Ah, thanks for the pointer! I tend to ignore anything that's not in the
man page. :)

Cheers, Phil

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Arturo Borrero Gonzalez]

On 28 April 2017 at 10:28, Phil Sutter <phil@nwl.cc> wrote:
> On Fri, Apr 28, 2017 at 10:11:51AM +0200, Arturo Borrero Gonzalez wrote:
>> On 28 April 2017 at 10:05, Phil Sutter <phil@nwl.cc> wrote:
>> >>
>> >> This warning will be printed even in rulesets loaded with '-f'
>> >> which first creates the masq rule an then the other chain.
>> >
>> > Hmm. I tested it with the following config and it works fine:
>> >
>> > | table ip nat {
>> > |       chain post {
>> > |               type nat hook postrouting priority 0; policy accept;
>> > |               oifname "veth2" masquerade
>> > |       }
>> > |
>> > |       chain pre {
>> > |               type nat hook prerouting priority 0; policy accept;
>> > |       }
>> > | }
>> >
>> > OK, with a config consisting of several 'add' commands, it indeed warns.
>> >
>> >> I think is just a matter of documenting *everywhere* that this is the
>> >> expected behaviour, not a bug.
>> >
>> > Yeah, I should indeed have done that first, also because masquerade
>> > statement is not documented at all yet.
>> >
>>
>> The best current documentation is this:
>>
>> https://wiki.nftables.org/wiki-nftables/index.php/Performing_Network_Address_Translation_(NAT)
>
> Ah, thanks for the pointer! I tend to ignore anything that's not in the
> man page. :)

Well, I guess adding more info to the man page won't hurt.

Things I would add:
 * some bits about NAT chains configuration (this issue)
 * info about base chains priorities
 * some bits about atomic operations

etc

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Pablo Neira Ayuso]

On Fri, Apr 28, 2017 at 11:02:53AM +0200, Arturo Borrero Gonzalez wrote:
> On 28 April 2017 at 10:28, Phil Sutter <phil@nwl.cc> wrote:
[...]
> > Ah, thanks for the pointer! I tend to ignore anything that's not in the
> > man page. :)
> 
> Well, I guess adding more info to the man page won't hurt.
> 
> Things I would add:
>  * some bits about NAT chains configuration (this issue)
>  * info about base chains priorities
>  * some bits about atomic operations

Either we update the existing documentation (manpage and wiki) as
Arturo suggest or we add some sort of transparent automatic NAT chain
registration in the kernel.

Re: [nft PATCH 2/2] masquerade: Complain if no prerouting chain exists

[Phil Sutter]

On Fri, Apr 28, 2017 at 04:58:01PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 28, 2017 at 11:02:53AM +0200, Arturo Borrero Gonzalez wrote:
> > On 28 April 2017 at 10:28, Phil Sutter <phil@nwl.cc> wrote:
> [...]
> > > Ah, thanks for the pointer! I tend to ignore anything that's not in the
> > > man page. :)
> > 
> > Well, I guess adding more info to the man page won't hurt.
> > 
> > Things I would add:
> >  * some bits about NAT chains configuration (this issue)
> >  * info about base chains priorities
> >  * some bits about atomic operations
> 
> Either we update the existing documentation (manpage and wiki) as
> Arturo suggest or we add some sort of transparent automatic NAT chain
> registration in the kernel.

I had considered that already, but didn't see an easy way to do it in
kernel space. Also, in order to do it properly, one would have to remove
it again if it's counterpart is removed, which further complicates
things. Hence my poor man's approach of just warning about it.

For the time being, I'll add a note to the man page pointing this out.
If it will become unnecessary in the future, it can still be removed.

Cheers, Phil
> --
> 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