| Message ID | 20170427132439.9443-2-phil@nwl.cc |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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. -- 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
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 -- 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
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)
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(-)