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

login
register
mail settings
Submitter Tomasz Bursztyka
Date May 14, 2013, 10:52 a.m.
Message ID <1368528727-10127-7-git-send-email-tomasz.bursztyka@linux.intel.com>
Download mbox | patch
Permalink /patch/243678/
State RFC
Headers show

Comments

Tomasz Bursztyka - May 14, 2013, 10:52 a.m.
Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 extensions/libipt_DNAT.c | 135 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 124 insertions(+), 11 deletions(-)
Pablo Neira - May 14, 2013, 10:30 p.m.
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.
--
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
Tomasz Bursztyka - May 15, 2013, 6:48 a.m.
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
--
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
Pablo Neira - May 15, 2013, 12:51 p.m.
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.
--
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
Tomasz Bursztyka - May 15, 2013, 1:24 p.m.
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
--
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
Pablo Neira - May 15, 2013, 1:49 p.m.
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.
--
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

Patch

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)