| Message ID | 1368528727-10127-7-git-send-email-tomasz.bursztyka@linux.intel.com |
|---|---|
| State | RFC |
| Headers | show |
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
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
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
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
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
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)
Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com> --- extensions/libipt_DNAT.c | 135 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 124 insertions(+), 11 deletions(-)