[iptables-nftables/libnfables PATCHES] Target translation to nftables

[iptables-nftables/libnfables PATCHES] Target translation to nftables

[Tomasz Bursztyka]

Hi,

You will find 2 sets of patches, one for libnftables and one 
foriptables-nftables.

Among some fixes, one feature has been added: DNAT support in 
iptables-nftables.

Iimplemented quickly only this one to show how to translate targets blob 
into nftables expression. Other ones can be easily implemented as well.
But whenit iseasy in that way, it will be a bit trickier in the other way.

Actually, I have not found a proper solution to output the rule in 
iptables format from the expression list. Nothing clean.
If someone has anice idea...


The 2 sets will follow this mail.

Br,

Tomasz

[libnftables PATCH 0/7] Fixes and features

[Tomasz Bursztyka]

patch 1, 2, 3, 4, 5 and 7 are various trivial fixes.
patch 6 is meant for expression list handling in iptables-nftables tool

Tomasz Bursztyka (7):
  git: add a .gitignore file
  build: add an autogen.sh script
  rule: declare nft_rule_list structure at a proper place
  expr: remove inconsistent and non implemented function
  map: fix nft_rule_expr_build_payload export
  expr: add support for expr list and capability to add it into a rule
  chain: handle attribute is relevant if only there is no name to use

 .gitignore                 | 30 ++++++++++++++++++++++++++++++
 autogen.sh                 |  4 ++++
 include/libnftables/expr.h |  7 ++++++-
 include/libnftables/rule.h |  5 +++++
 src/chain.c                |  3 ++-
 src/expr.c                 | 45 +++++++++++++++++++++++++++++++++++++++++++++
 src/internal.h             |  4 ++++
 src/libnftables.map        |  6 ++++++
 src/rule.c                 | 12 ++++++++++++
 9 files changed, 114 insertions(+), 2 deletions(-)
 create mode 100644 .gitignore
 create mode 100755 autogen.sh

-- 
1.8.2.1

[libnftables PATCH 1/7] git: add a .gitignore file

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 .gitignore | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 .gitignore

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..230e2a9
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,30 @@
+# Dependency and object files
+.*.d
+*.o
+*.lo
+*.la
+.deps
+.libs
+.dirstamp
+
+# Generated by autoconf/configure
+Makefile
+Makefile.in
+Makefile.defs
+Makefile.rules
+config.h
+config.h.in
+config.h.in~
+config.log
+config.status
+configure
+autom4te.cache
+stamp-h1
+aclocal.m4
+libnftables.pc
+libtool
+build-aux
+doxygen.cfg
+
+# Debian package build temporary files
+build-stamp
-- 
1.8.2.1

Re: [libnftables PATCH 1/7] git: add a .gitignore file

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:51:16PM +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  .gitignore | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 .gitignore
> 
> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 0000000..230e2a9
> --- /dev/null
> +++ b/.gitignore
> @@ -0,0 +1,30 @@
> +# Dependency and object files
> +.*.d
> +*.o
> +*.lo
> +*.la
> +.deps
> +.libs
> +.dirstamp
> +
> +# Generated by autoconf/configure
> +Makefile
> +Makefile.in
> +Makefile.defs
> +Makefile.rules

Please, consolidate this to Makefile*

> +config.h
> +config.h.in
> +config.h.in~
> +config.log
> +config.status

config.*

> +configure
> +autom4te.cache
> +stamp-h1
> +aclocal.m4
> +libnftables.pc
> +libtool
> +build-aux
> +doxygen.cfg
> +
> +# Debian package build temporary files
> +build-stamp
> -- 
> 1.8.2.1
> 
> --
> 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: [libnftables PATCH 1/7] git: add a .gitignore file

[Tomasz Bursztyka]

Hi Pablo,
>> +# Generated by autoconf/configure
>> +Makefile
>> +Makefile.in
>> +Makefile.defs
>> +Makefile.rules
> Please, consolidate this to Makefile*

You don't want to get rid of Makefile.am, do you? ;)

>> +config.h
>> +config.h.in
>> +config.h.in~
>> +config.log
>> +config.status
> config.*

True

Tomasz

Re: [libnftables PATCH 1/7] git: add a .gitignore file

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 09:51:50AM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> >>+# Generated by autoconf/configure
> >>+Makefile
> >>+Makefile.in
> >>+Makefile.defs
> >>+Makefile.rules
> >Please, consolidate this to Makefile*
> 
> You don't want to get rid of Makefile.am, do you? ;)

Right.

BTW, we don't seem to have Makefile.defs and Makefile.rules in
iptables.

Re: [libnftables PATCH 1/7] git: add a .gitignore file

[Tomasz Bursztyka]

Hi Pablo,
> BTW, we don't seem to have Makefile.defs and Makefile.rules in
> iptables.

It seem not indeed. I c/p this .gitignore from nftables project anyway.

Tomasz

[libnftables PATCH 2/7] build: add an autogen.sh script

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 autogen.sh | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100755 autogen.sh

diff --git a/autogen.sh b/autogen.sh
new file mode 100755
index 0000000..a0c4395
--- /dev/null
+++ b/autogen.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+autoreconf -fi;
+rm -Rf autom4te*.cache;
-- 
1.8.2.1

[libnftables PATCH 3/7] rule: declare nft_rule_list structure at a proper place

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 include/libnftables/rule.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/libnftables/rule.h b/include/libnftables/rule.h
index 50222c5..e7396a4 100644
--- a/include/libnftables/rule.h
+++ b/include/libnftables/rule.h
@@ -54,6 +54,8 @@ struct nft_rule_expr_iter *nft_rule_expr_iter_create(struct nft_rule *r);
 struct nft_rule_expr *nft_rule_expr_iter_next(struct nft_rule_expr_iter *iter);
 void nft_rule_expr_iter_destroy(struct nft_rule_expr_iter *iter);
 
+struct nft_rule_list;
+
 struct nft_rule_list *nft_rule_list_alloc(void);
 void nft_rule_list_free(struct nft_rule_list *list);
 void nft_rule_list_add(struct nft_rule *r, struct nft_rule_list *list);
-- 
1.8.2.1

[libnftables PATCH 4/7] expr: remove inconsistent and non implemented function

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 include/libnftables/expr.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
index 6d5d4e3..d899e41 100644
--- a/include/libnftables/expr.h
+++ b/include/libnftables/expr.h
@@ -27,8 +27,6 @@ uint32_t nft_rule_expr_get_u32(struct nft_rule_expr *expr, uint16_t type);
 uint64_t nft_rule_expr_get_u64(struct nft_rule_expr *expr, uint16_t type);
 const char *nft_rule_expr_get_str(struct nft_rule_expr *expr, uint16_t type);
 
-struct list_head *nft_rule_expr_list_head(struct nft_rule_expr *expr);
-
 void nft_rule_expr_build_payload(struct nlmsghdr *nlh, struct nft_rule_expr *expr);
 
 enum {
-- 
1.8.2.1

[libnftables PATCH 5/7] map: fix nft_rule_expr_build_payload export

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 src/libnftables.map | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/libnftables.map b/src/libnftables.map
index 957e3b6..3f98287 100644
--- a/src/libnftables.map
+++ b/src/libnftables.map
@@ -72,6 +72,7 @@ global:
   nft_rule_expr_get_u32;
   nft_rule_expr_get_u64;
   nft_rule_expr_get_str;
+  nft_rule_expr_build_payload;
   nft_rule_expr_free;
 
   nft_rule_list_alloc;
-- 
1.8.2.1

[libnftables PATCH 6/7] expr: add support for expr list and capability to add it into a rule

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 include/libnftables/expr.h |  7 +++++++
 include/libnftables/rule.h |  3 +++
 src/expr.c                 | 45 +++++++++++++++++++++++++++++++++++++++++++++
 src/internal.h             |  4 ++++
 src/libnftables.map        |  5 +++++
 src/rule.c                 | 12 ++++++++++++
 6 files changed, 76 insertions(+)

diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
index d899e41..4d789b4 100644
--- a/include/libnftables/expr.h
+++ b/include/libnftables/expr.h
@@ -27,6 +27,13 @@ uint32_t nft_rule_expr_get_u32(struct nft_rule_expr *expr, uint16_t type);
 uint64_t nft_rule_expr_get_u64(struct nft_rule_expr *expr, uint16_t type);
 const char *nft_rule_expr_get_str(struct nft_rule_expr *expr, uint16_t type);
 
+struct nft_rule_expr_list;
+
+struct nft_rule_expr_list *nft_rule_expr_list_alloc(void);
+void nft_rule_expr_list_free(struct nft_rule_expr_list *list);
+void nft_rule_expr_list_add(struct nft_rule_expr *expr, struct nft_rule_expr_list *list);
+void nft_rule_expr_list_add_list(struct nft_rule_expr_list *to_add, struct nft_rule_expr_list *list);
+
 void nft_rule_expr_build_payload(struct nlmsghdr *nlh, struct nft_rule_expr *expr);
 
 enum {
diff --git a/include/libnftables/rule.h b/include/libnftables/rule.h
index e7396a4..5c713ce 100644
--- a/include/libnftables/rule.h
+++ b/include/libnftables/rule.h
@@ -36,6 +36,9 @@ uint64_t nft_rule_attr_get_u64(struct nft_rule *r, uint16_t attr);
 
 void nft_rule_add_expr(struct nft_rule *r, struct nft_rule_expr *expr);
 
+struct nft_rule_expr_list;
+void nft_rule_add_expr_list(struct nft_rule *r, struct nft_rule_expr_list *list);
+
 void nft_rule_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_rule *t);
 
 enum {
diff --git a/src/expr.c b/src/expr.c
index 0b06aed..c73ac81 100644
--- a/src/expr.c
+++ b/src/expr.c
@@ -172,6 +172,51 @@ const char *nft_rule_expr_get_str(struct nft_rule_expr *expr, uint16_t type)
 }
 EXPORT_SYMBOL(nft_rule_expr_get_str);
 
+struct nft_rule_expr_list *nft_rule_expr_list_alloc(void)
+{
+	struct nft_rule_expr_list *list;
+
+	list = calloc(1, sizeof(struct nft_rule_expr_list));
+	if (list == NULL)
+		return NULL;
+
+	INIT_LIST_HEAD(&list->list);
+
+	return list;
+}
+EXPORT_SYMBOL(nft_rule_expr_list_alloc);
+
+void nft_rule_expr_list_free(struct nft_rule_expr_list *list)
+{
+	struct nft_rule_expr *e, *tmp;
+
+	list_for_each_entry_safe(e, tmp, &list->list, head) {
+		list_del(&e->head);
+		nft_rule_expr_free(e);
+	}
+	free(list);
+}
+EXPORT_SYMBOL(nft_rule_expr_list_free);
+
+void nft_rule_expr_list_add(struct nft_rule_expr *expr,
+			    struct nft_rule_expr_list *list)
+{
+	list_add_tail(&expr->head, &list->list);
+}
+EXPORT_SYMBOL(nft_rule_expr_list_add);
+
+void nft_rule_expr_list_add_list(struct nft_rule_expr_list *to_add,
+				 struct nft_rule_expr_list *list)
+{
+	struct nft_rule_expr *e, *tmp;
+
+	list_for_each_entry_safe(e, tmp, &to_add->list, head) {
+		list_del(&e->head);
+		list_add_tail(&e->head, &list->list);
+	}
+}
+EXPORT_SYMBOL(nft_rule_expr_list_add_list);
+
 void
 nft_rule_expr_build_payload(struct nlmsghdr *nlh, struct nft_rule_expr *expr)
 {
diff --git a/src/internal.h b/src/internal.h
index f5717ed..a93667e 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -22,6 +22,10 @@ struct nft_rule_expr {
 	uint8_t		data[];
 };
 
+struct nft_rule_expr_list {
+	struct list_head list;
+};
+
 struct nlattr;
 
 struct nft_set {
diff --git a/src/libnftables.map b/src/libnftables.map
index 3f98287..3cffb74 100644
--- a/src/libnftables.map
+++ b/src/libnftables.map
@@ -56,6 +56,7 @@ global:
   nft_rule_nlmsg_build_payload;
   nft_rule_nlmsg_parse;
   nft_rule_add_expr;
+  nft_rule_add_expr_list;
 
   nft_rule_expr_iter_create;
   nft_rule_expr_iter_next;
@@ -72,6 +73,10 @@ global:
   nft_rule_expr_get_u32;
   nft_rule_expr_get_u64;
   nft_rule_expr_get_str;
+  nft_rule_expr_list_alloc;
+  nft_rule_expr_list_free;
+  nft_rule_expr_list_add;
+  nft_rule_expr_list_add_list;
   nft_rule_expr_build_payload;
   nft_rule_expr_free;
 
diff --git a/src/rule.c b/src/rule.c
index 501b4f6..afc22d3 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -258,6 +258,18 @@ void nft_rule_add_expr(struct nft_rule *r, struct nft_rule_expr *expr)
 }
 EXPORT_SYMBOL(nft_rule_add_expr);
 
+void nft_rule_add_expr_list(struct nft_rule *r,
+			    struct nft_rule_expr_list *list)
+{
+	struct nft_rule_expr *e, *tmp;
+
+	list_for_each_entry_safe(e, tmp, &list->list, head) {
+		list_del(&e->head);
+		list_add_tail(&e->head, &r->expr_list);
+	}
+}
+EXPORT_SYMBOL(nft_rule_add_expr_list);
+
 static int nft_rule_parse_attr_cb(const struct nlattr *attr, void *data)
 {
 	const struct nlattr **tb = data;
-- 
1.8.2.1

[libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Tomasz Bursztyka]

While changing chain's settings, like its policy, it requires either the
handle or the name, but not both.

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 src/chain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/chain.c b/src/chain.c
index 1b1c3fe..e9a7896 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -263,7 +263,8 @@ void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
 		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
 		mnl_attr_nest_end(nlh, nest);
 	}
-	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
+	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
+	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
 		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));
 	if (c->flags & (1 << NFT_CHAIN_ATTR_TYPE))
 		mnl_attr_put_strz(nlh, NFTA_CHAIN_TYPE, c->type);
-- 
1.8.2.1

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:51:22PM +0300, Tomasz Bursztyka wrote:
> While changing chain's settings, like its policy, it requires either the
> handle or the name, but not both.
> 
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  src/chain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/chain.c b/src/chain.c
> index 1b1c3fe..e9a7896 100644
> --- a/src/chain.c
> +++ b/src/chain.c
> @@ -263,7 +263,8 @@ void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
>  		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
>  		mnl_attr_nest_end(nlh, nest);
>  	}
> -	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
> +	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
> +	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
>  		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));

The kernel will ignore the name if the handle is set. So no need to
make this artificial restriction in user-space.

>  	if (c->flags & (1 << NFT_CHAIN_ATTR_TYPE))
>  		mnl_attr_put_strz(nlh, NFTA_CHAIN_TYPE, c->type);
> -- 
> 1.8.2.1
> 
> --
> 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: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Tomasz Bursztyka]

Hi Pablo,
> On Tue, May 14, 2013 at 01:51:22PM +0300, Tomasz Bursztyka wrote:
>> While changing chain's settings, like its policy, it requires either the
>> handle or the name, but not both.
>>
>> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
>> ---
>>   src/chain.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/chain.c b/src/chain.c
>> index 1b1c3fe..e9a7896 100644
>> --- a/src/chain.c
>> +++ b/src/chain.c
>> @@ -263,7 +263,8 @@ void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
>>   		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
>>   		mnl_attr_nest_end(nlh, nest);
>>   	}
>> -	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
>> +	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
>> +	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
>>   		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));
> The kernel will ignore the name if the handle is set. So no need to
> make this artificial restriction in user-space.

No this not the case, have a look at net/netfilter/nf_tables_api.c in 
nf_tables_newchain(), lines 858-860:

if (nla[NFTA_CHAIN_HANDLE] && name &&
     !IS_ERR(nf_tables_chain_lookup(table, nla[NFTA_CHAIN_NAME])))
         return -EEXIST;

When handle and name are both present it means user wants to change the 
chain's name. (see line 882)
But in our case, when changing only the policy we don't touch the name, 
but libnftables provides it anyway thus failing on that test.||||

My patch is bogus anyway: I should add a marker that name has been 
changed first (and if only it was really different), and then handle it 
when building the message.


Tomasz

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 09:08:27AM +0300, Tomasz Bursztyka wrote:
[...]
> >The kernel will ignore the name if the handle is set. So no need to
> >make this artificial restriction in user-space.
> 
> No this not the case, have a look at net/netfilter/nf_tables_api.c
> in nf_tables_newchain(), lines 858-860:
> 
> if (nla[NFTA_CHAIN_HANDLE] && name &&
>     !IS_ERR(nf_tables_chain_lookup(table, nla[NFTA_CHAIN_NAME])))
>         return -EEXIST;
> 
> When handle and name are both present it means user wants to change
> the chain's name. (see line 882)
> But in our case, when changing only the policy we don't touch the
> name, but libnftables provides it anyway thus failing on that
> test.||||

But the handle number is built into the netlink message if the client
sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
only happens in nft_chain_user_rename.

This seems to me like the client needs to be fixed not to set both
attributes at the same time (unless it wants a chain rename).

Where are you hitting this?

> My patch is bogus anyway: I should add a marker that name has been
> changed first (and if only it was really different), and then handle
> it when building the message.

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Tomasz Bursztyka]

Hi Pablo,
> But the handle number is built into the netlink message if the client
> sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
> only happens in nft_chain_user_rename.
>
> This seems to me like the client needs to be fixed not to set both
> attributes at the same time (unless it wants a chain rename).
>
> Where are you hitting this?
>

I was actually playing on my own with libnftables.
It's easy: dump the chain list, then change the policy on one chain for 
instance, build the message to apply this change, send it...

We haven't hit the bug yet anywhere, because no code does such settings 
change after a dump, but we - or whatever app - surely will at some point.


Tomasz

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 04:06:26PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> >But the handle number is built into the netlink message if the client
> >sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
> >only happens in nft_chain_user_rename.
> >
> >This seems to me like the client needs to be fixed not to set both
> >attributes at the same time (unless it wants a chain rename).
> >
> >Where are you hitting this?
> >
> 
> I was actually playing on my own with libnftables.
> It's easy: dump the chain list, then change the policy on one chain
> for instance, build the message to apply this change, send it...
> 
> We haven't hit the bug yet anywhere, because no code does such
> settings change after a dump, but we - or whatever app - surely will
> at some point.

We should document that. I have a patch to add the doxygen doc to
libnftables. I prefer that user is in control of this, instead of
adding internal workarounds to avoid tricky situations.

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Tomasz Bursztyka]

Hi Pablo,
>> I was actually playing on my own with libnftables.
>> >It's easy: dump the chain list, then change the policy on one chain
>> >for instance, build the message to apply this change, send it...
>> >
>> >We haven't hit the bug yet anywhere, because no code does such
>> >settings change after a dump, but we - or whatever app - surely will
>> >at some point.
> We should document that. I have a patch to add the doxygen doc to
> libnftables. I prefer that user is in control of this, instead of
> adding internal workarounds to avoid tricky situations.

As you want, but it will complicate things for the user a bit.

Tomasz

Re: [libnftables PATCH 7/7] chain: handle attribute is relevant if only there is no name to use

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 04:54:46PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
>
> >>>I was actually playing on my own with libnftables.
> >>>It's easy: dump the chain list, then change the policy on one chain
> >>>for instance, build the message to apply this change, send it...
> >>>
> >>>We haven't hit the bug yet anywhere, because no code does such
> >>>settings change after a dump, but we - or whatever app - surely will
> >>>at some point.
> >
> >We should document that. I have a patch to add the doxygen doc to
> >libnftables. I prefer that user is in control of this, instead of
> >adding internal workarounds to avoid tricky situations.
> 
> As you want, but it will complicate things for the user a bit.

Going back to the kernel code (line 882):

       if (nla[NFTA_CHAIN_HANDLE] && name)
                nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);

We can check in the chain name in the kernel and the name passed are
the same, in that case we skip nla_strlcpy.

Re: [libnftables PATCH 0/7] Fixes and features

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:51:15PM +0300, Tomasz Bursztyka wrote:
> patch 1, 2, 3, 4, 5 and 7 are various trivial fixes.
> patch 6 is meant for expression list handling in iptables-nftables tool

Applied patches 1-5, I have amended patch 1 according to our
discussion. Wait for your new spin for patch 7.

Thanks Tomasz.

[iptables-nftables PATCH 0/6] Fixes and features

[Tomasz Bursztyka]

Small fixes here and there in patch 1, 2 and 3 

patch 4, 5, and 6 propose a way to translate iptables matches/targets blob (when needed) into
nftables expressions list. (thus it requires the expression list functions in libnftables) 

Tomasz Bursztyka (6):
  xtables: initialize xtables defaults even on listing rules
  xtables: destroy list iterator relevantly
  xtables: policy can be changed only on builtin chain
  xtables: Add support for translating xtables target into nft expressions
  xtables: add support for translating xtables matches into nft
    expressions
  xtables: add suport for DNAT rule translation to nft extensions

 configure.ac              |   7 +++
 extensions/GNUmakefile.in |   2 +-
 extensions/libipt_DNAT.c  | 135 ++++++++++++++++++++++++++++++++++++++++++----
 include/xtables.h.in      |   8 +++
 iptables/nft.c            |  71 +++++++++++++++---------
 5 files changed, 186 insertions(+), 37 deletions(-)

-- 
1.8.2.1

[iptables-nftables PATCH 1/6] xtables: initialize xtables defaults even on listing rules

[Tomasz Bursztyka]

Output of the tool should be the same as for iptables.

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index e68d112..f5d96bc 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2477,6 +2477,10 @@ int nft_rule_list(struct nft_handle *h, const char *chain, const char *table,
 	struct nft_chain_list_iter *iter;
 	struct nft_chain *c;
 
+	/* If built-in chains don't exist for this table, create them */
+	if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
+		nft_chain_builtin_init(h, table, NULL, NF_ACCEPT);
+
 	list = nft_chain_dump(h);
 
 	iter = nft_chain_list_iter_create(list);
-- 
1.8.2.1

Re: [iptables-nftables PATCH 1/6] xtables: initialize xtables defaults even on listing rules

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:52:02PM +0300, Tomasz Bursztyka wrote:
> Output of the tool should be the same as for iptables.

Applied, thanks Tomasz.

[iptables-nftables PATCH 2/6] xtables: destroy list iterator relevantly

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index f5d96bc..574383d 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1119,6 +1119,7 @@ next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
+	nft_chain_list_iter_destroy(iter);
 	nft_chain_list_free(list);
 
 	return 1;
@@ -1248,7 +1249,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table)
 {
 	int ret;
 	struct nft_chain_list *list;
-	struct nft_chain_list_iter *iter;
+	struct nft_chain_list_iter *iter = NULL;
 	struct nft_chain *c;
 
 	nft_fn = nft_rule_flush;
@@ -1285,6 +1286,7 @@ next:
 	}
 
 err:
+	nft_chain_list_iter_destroy(iter);
 	nft_chain_list_free(list);
 
 	/* the core expects 1 for success and 0 for error */
@@ -1356,7 +1358,7 @@ static bool nft_chain_builtin(struct nft_chain *c)
 int nft_chain_user_del(struct nft_handle *h, const char *chain, const char *table)
 {
 	struct nft_chain_list *list;
-	struct nft_chain_list_iter *iter;
+	struct nft_chain_list_iter *iter = NULL;
 	struct nft_chain *c;
 	int ret = 0;
 	int deleted_ctr = 0;
@@ -1398,6 +1400,7 @@ next:
 	}
 
 err:
+	nft_chain_list_iter_destroy(iter);
 	nft_chain_list_free(list);
 
 	/* chain not found */
@@ -1580,6 +1583,7 @@ bool nft_table_find(struct nft_handle *h, const char *tablename)
 		t = nft_table_list_iter_next(iter);
 	}
 
+	nft_table_list_iter_destroy(iter);
 	nft_table_list_free(list);
 
 err:
@@ -1617,6 +1621,7 @@ int nft_for_each_table(struct nft_handle *h,
 		t = nft_table_list_iter_next(iter);
 	}
 
+	nft_table_list_iter_destroy(iter);
 	nft_table_list_free(list);
 
 err:
@@ -2521,6 +2526,7 @@ next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
+	nft_chain_list_iter_destroy(iter);
 	nft_chain_list_free(list);
 
 	return 1;
@@ -2575,6 +2581,7 @@ next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
+	nft_chain_list_iter_destroy(iter);
 	return 1;
 }
 
@@ -2615,6 +2622,7 @@ next:
 		c = nft_chain_list_iter_next(iter);
 	}
 
+	nft_chain_list_iter_destroy(iter);
 	nft_chain_list_free(list);
 
 	return 1;
@@ -2821,6 +2829,7 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 		xtables_config_perror(flags, "table `%s' has been created\n",
 			(char *)nft_table_attr_get(table, NFT_TABLE_ATTR_NAME));
 	}
+	nft_table_list_iter_destroy(titer);
 
 	/* Stage 2) create chains */
 	citer = nft_chain_list_iter_create(chain_list);
@@ -2846,5 +2855,6 @@ int nft_xtables_config_load(struct nft_handle *h, const char *filename,
 			(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_NAME),
 			(char *)nft_chain_attr_get(chain, NFT_CHAIN_ATTR_TABLE));
 	}
+	nft_chain_list_iter_destroy(citer);
 	return 0;
 }
-- 
1.8.2.1

Re: [iptables-nftables PATCH 2/6] xtables: destroy list iterator relevantly

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:52:03PM +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  iptables/nft.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index f5d96bc..574383d 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -1119,6 +1119,7 @@ next:
>  		c = nft_chain_list_iter_next(iter);
>  	}
>  
> +	nft_chain_list_iter_destroy(iter);
>  	nft_chain_list_free(list);
>  
>  	return 1;
> @@ -1248,7 +1249,7 @@ int nft_rule_flush(struct nft_handle *h, const char *chain, const char *table)
>  {
>  	int ret;
>  	struct nft_chain_list *list;
> -	struct nft_chain_list_iter *iter;
> +	struct nft_chain_list_iter *iter = NULL;

By looking at the code, we can skip setting this to NULL.

>  	struct nft_chain *c;
>  
>  	nft_fn = nft_rule_flush;
> @@ -1285,6 +1286,7 @@ next:
>  	}
>  
>  err:
> +	nft_chain_list_iter_destroy(iter);

By moving this before the err: label. I'm going to take over this
patch and give it another spin.

[iptables-nftables PATCH 3/6] xtables: policy can be changed only on builtin chain

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 iptables/nft.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 574383d..afbba84 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -503,16 +503,9 @@ __nft_chain_set(struct nft_handle *h, const char *table,
 		c = nft_chain_builtin_alloc(_t, _c, policy);
 		if (c == NULL)
 			return -1;
-
 	} else {
-		/* This is a custom chain */
-		c = nft_chain_alloc();
-		if (c == NULL)
-			return -1;
-
-		nft_chain_attr_set(c, NFT_CHAIN_ATTR_TABLE, (char *)table);
-		nft_chain_attr_set(c, NFT_CHAIN_ATTR_NAME, (char *)chain);
-		nft_chain_attr_set_u32(c, NFT_CHAIN_ATTR_POLICY, policy);
+		errno = ENOENT;
+		return -1;
 	}
 
 	if (counters) {
-- 
1.8.2.1

Re: [iptables-nftables PATCH 3/6] xtables: policy can be changed only on builtin chain

[Pablo Neira Ayuso]

Also applied, thanks.

[iptables-nftables PATCH 4/6] xtables: Add support for translating xtables target into nft expressions

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 configure.ac              |  7 +++++++
 extensions/GNUmakefile.in |  2 +-
 include/xtables.h.in      |  5 +++++
 iptables/nft.c            | 21 ++++++++++++++-------
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 48a0d54..e228078 100644
--- a/configure.ac
+++ b/configure.ac
@@ -104,6 +104,13 @@ PKG_CHECK_MODULES([libnftables], [libnftables >= 1.0],
 	[nftables=1], [nftables=0])
 AM_CONDITIONAL([HAVE_LIBNFTABLES], [test "$nftables" = 1])
 
+if test "$nftables" = 1; then
+	EXTENSION_NFT_LDFLAGS="${libmnl_LIBS} ${libnftables_LIBS}";
+else
+	EXTENSION_NFT_LDFLAGS="";
+fi;
+AC_SUBST(EXTENSION_NFT_LDFLAGS)
+
 AM_PROG_LEX
 AC_PROG_YACC
 
diff --git a/extensions/GNUmakefile.in b/extensions/GNUmakefile.in
index 4a8ff49..28034d7 100644
--- a/extensions/GNUmakefile.in
+++ b/extensions/GNUmakefile.in
@@ -91,7 +91,7 @@ init%.o: init%.c
 #	Shared libraries
 #
 lib%.so: lib%.oo
-	${AM_VERBOSE_CCLD} ${CCLD} ${AM_LDFLAGS} -shared ${LDFLAGS} -o $@ $< -L../libxtables/.libs -lxtables ${$*_LIBADD};
+	${AM_VERBOSE_CCLD} ${CCLD} ${AM_LDFLAGS} -shared ${LDFLAGS} -o $@ $< -L../libxtables/.libs -lxtables ${$*_LIBADD} @EXTENSION_NFT_LDFLAGS@;
 
 lib%.oo: ${srcdir}/lib%.c
 	${AM_VERBOSE_CC} ${CC} ${AM_CPPFLAGS} ${AM_DEPFLAGS} ${AM_CFLAGS} -D_INIT=lib$*_init -DPIC -fPIC ${CFLAGS} -o $@ -c $<;
diff --git a/include/xtables.h.in b/include/xtables.h.in
index 10b241f..74df61f 100644
--- a/include/xtables.h.in
+++ b/include/xtables.h.in
@@ -18,6 +18,8 @@
 #include <linux/netfilter.h>
 #include <linux/netfilter/x_tables.h>
 
+#include <libnftables/expr.h>
+
 #ifndef IPPROTO_SCTP
 #define IPPROTO_SCTP 132
 #endif
@@ -327,6 +329,9 @@ struct xtables_target
 	void (*x6_fcheck)(struct xt_fcheck_call *);
 	const struct xt_option_entry *x6_options;
 
+	/* NFT related */
+	struct nft_rule_expr_list *(*translate_to_nft)(struct xt_entry_target *);
+
 	size_t udata_size;
 
 	/* Ignore these men behind the curtain: */
diff --git a/iptables/nft.c b/iptables/nft.c
index afbba84..341d092 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -593,16 +593,23 @@ static void __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
 	nft_rule_expr_set(e, NFT_EXPR_TG_INFO, info, t->u.target_size - sizeof(*t));
 }
 
-static void add_target(struct nft_rule *r, struct xt_entry_target *t)
+static void add_target(struct nft_rule *r, struct xtables_target *target)
 {
 	struct nft_rule_expr *expr;
+	struct nft_rule_expr_list *expr_list;
 
-	expr = nft_rule_expr_alloc("target");
-	if (expr == NULL)
-		return;
+	if (target->translate_to_nft == NULL) {
+		expr = nft_rule_expr_alloc("target");
+		if (expr == NULL)
+			return;
 
-	__add_target(expr, t);
-	nft_rule_add_expr(r, expr);
+		__add_target(expr, target->t);
+		nft_rule_add_expr(r, expr);
+	} else {
+		expr_list = target->translate_to_nft(target->t);
+		if (expr_list != NULL)
+			nft_rule_add_expr_list(r, expr_list);
+	}
 }
 
 static void add_jumpto(struct nft_rule *r, const char *name, int verdict)
@@ -712,7 +719,7 @@ nft_rule_add(struct nft_handle *h, const char *chain, const char *table,
 		else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
 			add_verdict(r, NFT_RETURN);
 		else
-			add_target(r, cs->target->t);
+			add_target(r, cs->target);
 	} else if (strlen(cs->jumpto) > 0) {
 		/* Not standard, then it's a go / jump to chain */
 		if (ip_flags & IPT_F_GOTO)
-- 
1.8.2.1

[iptables-nftables PATCH 5/6] xtables: add support for translating xtables matches into nft expressions

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 include/xtables.h.in |  3 +++
 iptables/nft.c       | 21 ++++++++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/xtables.h.in b/include/xtables.h.in
index 74df61f..407c382 100644
--- a/include/xtables.h.in
+++ b/include/xtables.h.in
@@ -261,6 +261,9 @@ struct xtables_match
 	void (*x6_fcheck)(struct xt_fcheck_call *);
 	const struct xt_option_entry *x6_options;
 
+	/* NFT related */
+	struct nft_rule_expr_list *(*translate_to_nft)(struct xt_entry_match *);
+
 	/* Size of per-extension instance extra "global" scratch space */
 	size_t udata_size;
 
diff --git a/iptables/nft.c b/iptables/nft.c
index 341d092..70aa1f8 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -562,16 +562,23 @@ static void __add_match(struct nft_rule_expr *e, struct xt_entry_match *m)
 	nft_rule_expr_set(e, NFT_EXPR_MT_INFO, info, m->u.match_size - sizeof(*m));
 }
 
-static void add_match(struct nft_rule *r, struct xt_entry_match *m)
+static void add_match(struct nft_rule *r, struct xtables_match *match)
 {
 	struct nft_rule_expr *expr;
+	struct nft_rule_expr_list *expr_list;
 
-	expr = nft_rule_expr_alloc("match");
-	if (expr == NULL)
-		return;
+	if (match->translate_to_nft == NULL) {
+		expr = nft_rule_expr_alloc("match");
+		if (expr == NULL)
+			return;
 
-	__add_match(expr, m);
-	nft_rule_add_expr(r, expr);
+		__add_match(expr, match->m);
+		nft_rule_add_expr(r, expr);
+	} else {
+		expr_list = match->translate_to_nft(match->m);
+		if (expr_list != NULL)
+			nft_rule_add_expr_list(r, expr_list);
+	}
 }
 
 static void __add_target(struct nft_rule_expr *e, struct xt_entry_target *t)
@@ -702,7 +709,7 @@ nft_rule_add(struct nft_handle *h, const char *chain, const char *table,
 	ip_flags = h->ops->add(r, cs);
 
 	for (matchp = cs->matches; matchp; matchp = matchp->next)
-		add_match(r, matchp->match->m);
+		add_match(r, matchp->match);
 
 	/* Counters need to me added before the target, otherwise they are
 	 * increased for each rule because of the way nf_tables works.
-- 
1.8.2.1

[iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Tomasz Bursztyka]

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 extensions/libipt_DNAT.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 124 insertions(+), 11 deletions(-)

diff --git a/extensions/libipt_DNAT.c b/extensions/libipt_DNAT.c
index 466c9de..1d397fe 100644
--- a/extensions/libipt_DNAT.c
+++ b/extensions/libipt_DNAT.c
@@ -7,6 +7,7 @@
 #include <limits.h> /* INT_MAX in ip_tables.h */
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <net/netfilter/nf_nat.h>
+#include <linux/netfilter/nf_tables.h>
 
 enum {
 	O_TO_DEST = 0,
@@ -242,18 +243,130 @@ static void DNAT_save(const void *ip, const struct xt_entry_target *target)
 	}
 }
 
+static struct nft_rule_expr_list *
+add_nat_data(struct nft_rule_expr_list *expr_list, int reg, uint32_t data)
+{
+	struct nft_rule_expr *expr;
+
+	expr = nft_rule_expr_alloc("immediate");
+	if (expr == NULL)
+		return NULL;
+
+	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DREG, reg);
+	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DATA, data);
+
+	nft_rule_expr_list_add(expr, expr_list);
+
+	return expr_list;
+}
+
+static struct nft_rule_expr_list *
+create_nat_expr_list(const struct nf_nat_range *r)
+{
+	struct nft_rule_expr_list *expr_list;
+	struct nft_rule_expr *nat_expr;
+	int registers = 1;
+
+	expr_list = nft_rule_expr_list_alloc();
+	if (expr_list == NULL)
+		return NULL;
+
+	nat_expr = nft_rule_expr_alloc("nat");
+	if (nat_expr == NULL)
+		goto err;
+
+	nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_TYPE, NFT_NAT_DNAT);
+	nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_FAMILY, AF_INET);
+
+	if (r->flags & IP_NAT_RANGE_MAP_IPS) {
+		nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_REG_ADDR_MIN,
+				      registers);
+		if (add_nat_data(expr_list, registers, r->min_ip) == NULL)
+			goto err;
+		registers++;
+
+		if (r->max_ip != r->min_ip) {
+			nft_rule_expr_set_u32(nat_expr,
+					      NFT_EXPR_NAT_REG_ADDR_MAX,
+					      registers);
+			if (add_nat_data(expr_list,
+					 registers, r->max_ip) == NULL)
+				goto err;
+			registers++;
+		}
+	}
+
+	if (r->flags & IP_NAT_RANGE_PROTO_SPECIFIED) {
+		nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_REG_PROTO_MIN,
+				      registers);
+		if (add_nat_data(expr_list, registers,
+				 ntohs(r->min.tcp.port)) == NULL)
+			goto err;
+		registers++;
+
+		if (r->max.tcp.port != r->min.tcp.port) {
+			nft_rule_expr_set_u32(nat_expr,
+					      NFT_EXPR_NAT_REG_PROTO_MAX,
+					      registers);
+			if (add_nat_data(expr_list, registers,
+					 ntohs(r->max.tcp.port)) == NULL)
+				goto err;
+		}
+	}
+
+	nft_rule_expr_list_add(nat_expr, expr_list);
+	return expr_list;
+err:
+	nft_rule_expr_list_free(expr_list);
+
+	if (nat_expr != NULL)
+		nft_rule_expr_free(nat_expr);
+
+	return NULL;
+}
+
+static struct nft_rule_expr_list *DNAT_to_nft(struct xt_entry_target *target)
+{
+	const struct ipt_natinfo *info = (const void *)target;
+	struct nft_rule_expr_list *nat_expr_list;
+	int i;
+
+	nat_expr_list = nft_rule_expr_list_alloc();
+	if (nat_expr_list == NULL)
+		goto err;
+
+	for (i = 0; i < info->mr.rangesize; i++) {
+		struct nft_rule_expr_list *nat_expr;
+
+		nat_expr = create_nat_expr_list(&info->mr.range[i]);
+		if (nat_expr == NULL)
+			goto err;
+
+		nft_rule_expr_list_add_list(nat_expr, nat_expr_list);
+	}
+
+	return nat_expr_list;
+
+err:
+	if (nat_expr_list != NULL)
+		nft_rule_expr_list_free(nat_expr_list);
+
+	return NULL;
+}
+
 static struct xtables_target dnat_tg_reg = {
-	.name		= "DNAT",
-	.version	= XTABLES_VERSION,
-	.family		= NFPROTO_IPV4,
-	.size		= XT_ALIGN(sizeof(struct nf_nat_multi_range)),
-	.userspacesize	= XT_ALIGN(sizeof(struct nf_nat_multi_range)),
-	.help		= DNAT_help,
-	.x6_parse	= DNAT_parse,
-	.x6_fcheck	= DNAT_fcheck,
-	.print		= DNAT_print,
-	.save		= DNAT_save,
-	.x6_options	= DNAT_opts,
+	.name		  = "DNAT",
+	.version	  = XTABLES_VERSION,
+	.family		  = NFPROTO_IPV4,
+	.size		  = XT_ALIGN(sizeof(struct nf_nat_multi_range)),
+	.userspacesize	  = XT_ALIGN(sizeof(struct nf_nat_multi_range)),
+	.help		  = DNAT_help,
+	.x6_parse	  = DNAT_parse,
+	.x6_fcheck	  = DNAT_fcheck,
+	.print		  = DNAT_print,
+	.save		  = DNAT_save,
+	.x6_options	  = DNAT_opts,
+	.translate_to_nft = DNAT_to_nft,
 };
 
 void _init(void)
-- 
1.8.2.1

Re: [iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Pablo Neira Ayuso]

On Tue, May 14, 2013 at 01:52:07PM +0300, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  extensions/libipt_DNAT.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 124 insertions(+), 11 deletions(-)
> 
> diff --git a/extensions/libipt_DNAT.c b/extensions/libipt_DNAT.c
> index 466c9de..1d397fe 100644
> --- a/extensions/libipt_DNAT.c
> +++ b/extensions/libipt_DNAT.c
> @@ -7,6 +7,7 @@
>  #include <limits.h> /* INT_MAX in ip_tables.h */
>  #include <linux/netfilter_ipv4/ip_tables.h>
>  #include <net/netfilter/nf_nat.h>
> +#include <linux/netfilter/nf_tables.h>
>  
>  enum {
>  	O_TO_DEST = 0,
> @@ -242,18 +243,130 @@ static void DNAT_save(const void *ip, const struct xt_entry_target *target)
>  	}
>  }
>  
> +static struct nft_rule_expr_list *
> +add_nat_data(struct nft_rule_expr_list *expr_list, int reg, uint32_t data)
> +{
> +	struct nft_rule_expr *expr;
> +
> +	expr = nft_rule_expr_alloc("immediate");
> +	if (expr == NULL)
> +		return NULL;
> +
> +	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DREG, reg);
> +	nft_rule_expr_set_u32(expr, NFT_EXPR_IMM_DATA, data);
> +
> +	nft_rule_expr_list_add(expr, expr_list);
> +
> +	return expr_list;
> +}
> +
> +static struct nft_rule_expr_list *
> +create_nat_expr_list(const struct nf_nat_range *r)
> +{
> +	struct nft_rule_expr_list *expr_list;
> +	struct nft_rule_expr *nat_expr;
> +	int registers = 1;
> +
> +	expr_list = nft_rule_expr_list_alloc();
> +	if (expr_list == NULL)
> +		return NULL;

Better allocate this list in nft.c and pass it as parameter. All
extensions will require this, and after that change you can return -1
on error / 0 on success.

Or simply pass the struct nft_rule object? Then, you can skip patch
[libnftables PATCH 6/7]?

> +	nat_expr = nft_rule_expr_alloc("nat");
> +	if (nat_expr == NULL)
> +		goto err;
> +
> +	nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_TYPE, NFT_NAT_DNAT);
> +	nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_FAMILY, AF_INET);
> +
> +	if (r->flags & IP_NAT_RANGE_MAP_IPS) {
> +		nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_REG_ADDR_MIN,
> +				      registers);
> +		if (add_nat_data(expr_list, registers, r->min_ip) == NULL)
> +			goto err;
> +		registers++;
> +
> +		if (r->max_ip != r->min_ip) {
> +			nft_rule_expr_set_u32(nat_expr,
> +					      NFT_EXPR_NAT_REG_ADDR_MAX,
> +					      registers);
> +			if (add_nat_data(expr_list,
> +					 registers, r->max_ip) == NULL)
> +				goto err;
> +			registers++;
> +		}
> +	}
> +
> +	if (r->flags & IP_NAT_RANGE_PROTO_SPECIFIED) {
> +		nft_rule_expr_set_u32(nat_expr, NFT_EXPR_NAT_REG_PROTO_MIN,
> +				      registers);
> +		if (add_nat_data(expr_list, registers,
> +				 ntohs(r->min.tcp.port)) == NULL)
> +			goto err;
> +		registers++;
> +
> +		if (r->max.tcp.port != r->min.tcp.port) {
> +			nft_rule_expr_set_u32(nat_expr,
> +					      NFT_EXPR_NAT_REG_PROTO_MAX,
> +					      registers);
> +			if (add_nat_data(expr_list, registers,
> +					 ntohs(r->max.tcp.port)) == NULL)
> +				goto err;
> +		}
> +	}
> +
> +	nft_rule_expr_list_add(nat_expr, expr_list);
> +	return expr_list;
> +err:
> +	nft_rule_expr_list_free(expr_list);
> +
> +	if (nat_expr != NULL)
> +		nft_rule_expr_free(nat_expr);
> +
> +	return NULL;
> +}
> +
> +static struct nft_rule_expr_list *DNAT_to_nft(struct xt_entry_target *target)
> +{
> +	const struct ipt_natinfo *info = (const void *)target;
> +	struct nft_rule_expr_list *nat_expr_list;
> +	int i;
> +
> +	nat_expr_list = nft_rule_expr_list_alloc();
> +	if (nat_expr_list == NULL)
> +		goto err;
> +
> +	for (i = 0; i < info->mr.rangesize; i++) {
> +		struct nft_rule_expr_list *nat_expr;
> +
> +		nat_expr = create_nat_expr_list(&info->mr.range[i]);
> +		if (nat_expr == NULL)
> +			goto err;
> +
> +		nft_rule_expr_list_add_list(nat_expr, nat_expr_list);
> +	}
> +
> +	return nat_expr_list;
> +
> +err:
> +	if (nat_expr_list != NULL)
> +		nft_rule_expr_list_free(nat_expr_list);
> +
> +	return NULL;
> +}
> +
>  static struct xtables_target dnat_tg_reg = {
> -	.name		= "DNAT",
> -	.version	= XTABLES_VERSION,
> -	.family		= NFPROTO_IPV4,
> -	.size		= XT_ALIGN(sizeof(struct nf_nat_multi_range)),
> -	.userspacesize	= XT_ALIGN(sizeof(struct nf_nat_multi_range)),
> -	.help		= DNAT_help,
> -	.x6_parse	= DNAT_parse,
> -	.x6_fcheck	= DNAT_fcheck,
> -	.print		= DNAT_print,
> -	.save		= DNAT_save,
> -	.x6_options	= DNAT_opts,
> +	.name		  = "DNAT",
> +	.version	  = XTABLES_VERSION,
> +	.family		  = NFPROTO_IPV4,
> +	.size		  = XT_ALIGN(sizeof(struct nf_nat_multi_range)),
> +	.userspacesize	  = XT_ALIGN(sizeof(struct nf_nat_multi_range)),
> +	.help		  = DNAT_help,
> +	.x6_parse	  = DNAT_parse,
> +	.x6_fcheck	  = DNAT_fcheck,
> +	.print		  = DNAT_print,
> +	.save		  = DNAT_save,
> +	.x6_options	  = DNAT_opts,
> +	.translate_to_nft = DNAT_to_nft,

nft_to_translate is missing, right? We need it to print the rule that
is expressed in native format.

Probably you can call this xt_to_nft or struct_to_nft? It would be
shorter and won't require realigning dnat_tg_reg I would like to skip
those to avoid possible conflicts when merging this, we have more than
100 extensions.

BTW, some short description on the patches is a good idea, a couple of
lines description the intention after this (I know well what you're
making but others may not).

Thanks.

Re: [iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Tomasz Bursztyka]

Hi Pablo,
>> +static struct nft_rule_expr_list *
>> +create_nat_expr_list(const struct nf_nat_range *r)
>> +{
>> +	struct nft_rule_expr_list *expr_list;
>> +	struct nft_rule_expr *nat_expr;
>> +	int registers = 1;
>> +
>> +	expr_list = nft_rule_expr_list_alloc();
>> +	if (expr_list == NULL)
>> +		return NULL;
> Better allocate this list in nft.c and pass it as parameter. All
> extensions will require this, and after that change you can return -1
> on error / 0 on success.
>
> Or simply pass the struct nft_rule object? Then, you can skip patch
> [libnftables PATCH 6/7]?

Why not, it's a design preference. I liked the idea extension don't mess 
up with the rule and only provides its expression list.
it's less code on libnftables on your idea at least.

>> +	.x6_options	  = DNAT_opts,
>> +	.translate_to_nft = DNAT_to_nft,
> nft_to_translate is missing, right? We need it to print the rule that
> is expressed in native format.

Read the very first mail of this thread. It's actually an issue here.
I had the idea of doing the reverse function indeed, but there is a problem:
 From iptables to nftables it's easy, since we get a target made by the 
right extension, so we directly get the right translation function. 
That's what I did.

Now on the reverse way, we don't know at all to which extension the 
expression list belongs to, so which translation function to call.
Currently the only way I see it is to loop on all extensions until one 
returns successfully.
We should take care of the position in the expression list as well, and 
here I see we will need some more functions from libnftables.

I will try a PoC

> Probably you can call this xt_to_nft or struct_to_nft? It would be
> shorter and won't require realigning dnat_tg_reg I would like to skip
> those to avoid possible conflicts when merging this, we have more than
> 100 extensions.
>
> BTW, some short description on the patches is a good idea, a couple of
> lines description the intention after this (I know well what you're
> making but others may not).

Sure.


Tomasz

Re: [iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Pablo Neira Ayuso]

Hi Tomasz,

On Wed, May 15, 2013 at 09:48:28AM +0300, Tomasz Bursztyka wrote:
[...]
> >>+static struct nft_rule_expr_list *
> >>+create_nat_expr_list(const struct nf_nat_range *r)
> >>+{
> >>+	struct nft_rule_expr_list *expr_list;
> >>+	struct nft_rule_expr *nat_expr;
> >>+	int registers = 1;
> >>+
> >>+	expr_list = nft_rule_expr_list_alloc();
> >>+	if (expr_list == NULL)
> >>+		return NULL;
> >Better allocate this list in nft.c and pass it as parameter. All
> >extensions will require this, and after that change you can return -1
> >on error / 0 on success.
> >
> >Or simply pass the struct nft_rule object? Then, you can skip patch
> >[libnftables PATCH 6/7]?
> 
> Why not, it's a design preference. I liked the idea extension don't
> mess up with the rule and only provides its expression list.
> it's less code on libnftables on your idea at least.

We have to trust our iptables extensions.

What extra sanity checking are you going to make anyway if the
extension puzzles with this internal expr_list?

> >>+	.x6_options	  = DNAT_opts,
> >>+	.translate_to_nft = DNAT_to_nft,
> >nft_to_translate is missing, right? We need it to print the rule that
> >is expressed in native format.
> 
> Read the very first mail of this thread. It's actually an issue here.
> I had the idea of doing the reverse function indeed, but there is a problem:
> From iptables to nftables it's easy, since we get a target made by
> the right extension, so we directly get the right translation
> function. That's what I did.
>
> Now on the reverse way, we don't know at all to which extension the
> expression list belongs to, so which translation function to call.
> Currently the only way I see it is to loop on all extensions until
> one returns successfully.

You need some dispatcher code that interprets the nft_expr and routes
it to the right iptables extension. So you will need also one .c file
per expression in the kernel, e.g. nft_nat.c, that performs this
dispatching / routing to the right extension.

Probably checking netlink_delinearize.c in nft can provide your some
ideas.

> We should take care of the position in the expression list as well,
> and here I see we will need some more functions from libnftables.

You have the expression iterator already.

Re: [iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Tomasz Bursztyka]

Hi Pablo,
>> Why not, it's a design preference. I liked the idea extension don't
>> mess up with the rule and only provides its expression list.
>> it's less code on libnftables on your idea at least.
> We have to trust our iptables extensions.
>
> What extra sanity checking are you going to make anyway if the
> extension puzzles with this internal expr_list?

What do you mean?

Return 0/-1 instead of pointer/NULL is same.
As I said I am fine with your proposal in addition that it requires less 
code in libnftables.

>> Now on the reverse way, we don't know at all to which extension the
>> expression list belongs to, so which translation function to call.
>> Currently the only way I see it is to loop on all extensions until
>> one returns successfully.
> You need some dispatcher code that interprets the nft_expr and routes
> it to the right iptables extension. So you will need also one .c file
> per expression in the kernel, e.g. nft_nat.c, that performs this
> dispatching / routing to the right extension.

You lost me. Why kernel is involved here?

> Probably checking netlink_delinearize.c in nft can provide your some
> ideas.

Yes, and I actually use netlink_linearize.c to help for translation.

>> We should take care of the position in the expression list as well,
>> and here I see we will need some more functions from libnftables.
> You have the expression iterator already.

I believe it won't be sufficient. Let's see.


Tomasz

Re: [iptables-nftables PATCH 6/6] xtables: add suport for DNAT rule translation to nft extensions

[Pablo Neira Ayuso]

On Wed, May 15, 2013 at 04:24:04PM +0300, Tomasz Bursztyka wrote:
[...]
> >>Now on the reverse way, we don't know at all to which extension the
> >>expression list belongs to, so which translation function to call.
> >>Currently the only way I see it is to loop on all extensions until
> >>one returns successfully.
>
> >You need some dispatcher code that interprets the nft_expr and routes
> >it to the right iptables extension. So you will need also one .c file
> >per expression in the kernel, e.g. nft_nat.c, that performs this
> >dispatching / routing to the right extension.
> 
> You lost me. Why kernel is involved here?

I didn't mention the kernel is involved this.

You can have a dispatcher like:

static const struct {
        const char      *name;
        void            (*parse)(struct nft_rule_expr_iter *iter)
} netlink_parsers[] = {
        [...]
        { .name = "nat",        .parse = netlink_parse_nat },
};

the .parse callback gets an iterator to obtain the current expression
and munch more of them if required (will be useful for the payload
case).

Then, the netlink_parse_nat will route the nft_rule_expr object to the
corresponding libxt extension.

> >Probably checking netlink_delinearize.c in nft can provide your some
> >ideas.
> 
> Yes, and I actually use netlink_linearize.c to help for translation.
> 
> >>We should take care of the position in the expression list as well,
> >>and here I see we will need some more functions from libnftables.
> >You have the expression iterator already.
> 
> I believe it won't be sufficient. Let's see.

OK, let's revisit this once you hit limitations.