| Message ID | 1376055090-26551-12-git-send-email-tomasz.bursztyka@linux.intel.com |
|---|---|
| State | RFC |
| Headers | show |
On Fri, Aug 09, 2013 at 04:31:25PM +0300, Tomasz Bursztyka wrote: > Now that we parse properly, in one place and at once, the rule back into > a command structure, it's now easier to print the rule from that command > structure. I like patches from 11 to 13, that refactorization save us quite some code and the result in one single parsing and we also work with the command structure. Please, can you send me these three patches in first place? -- 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, > I like patches from 11 to 13, that refactorization save us quite some > code and the result in one single parsing and we also work with the > command structure. > > Please, can you send me these three patches in first place? Not impossible but means a big rebase of this patchset. (it currently needs translation stuff, or then it would require some work to handle target/matches properly as original code does not in nft_rule_to_iptables_command_state, without solving pure-nft expressed extensions of course) Could you recheck in detail patches 3/16 and 4/16? If there is no flaws in the translation engine, we might just go for it at it is. Thanks, 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 Mon, Aug 12, 2013 at 10:54:27AM +0300, Tomasz Bursztyka wrote: > Hi Pablo, > > >I like patches from 11 to 13, that refactorization save us quite some > >code and the result in one single parsing and we also work with the > >command structure. > > > >Please, can you send me these three patches in first place? > > Not impossible but means a big rebase of this patchset. > (it currently needs translation stuff, or then it would require some > work to handle target/matches properly as original code does not in > nft_rule_to_iptables_command_state, without solving pure-nft > expressed extensions of course) > > Could you recheck in detail patches 3/16 and 4/16? > If there is no flaws in the translation engine, we might just go for > it at it is. I'm not convinced that this approach is the way to go. In the payload case, the number of instruction tuples will explode, and you will have to iterate many times to find the corresponding parser. I started a quick patch this weekend based on this, the initial idea is the following: 1) Assuming we use a common path to translate rules from nft to xt in nft_rule_to_iptables_command_state, that function invokes a parser depending on the instruction: +void nft_parse_nat(struct nft_parse_ctx *ctx, struct nft_rule_expr *e, + struct nft_rule_expr_iter *iter, int family, + struct iptables_command_state *cs) +{ + int nat_type; + struct nf_nat_range range; + uint32_t reg; + + nat_type = nft_rule_expr_get_u32(e, NFT_EXPR_NAT_TYPE); + + reg = nft_rule_expr_get_u32(e, NFT_EXPR_NAT_REG_ADDR_MIN); + if (reg) + range.min_addr.ip = ctx->registers[reg]; + + reg = nft_rule_expr_get_u32(e, NFT_EXPR_NAT_REG_ADDR_MAX); + if (reg) { + range.flags |= NF_NAT_RANGE_MAP_IPS; + range.max_addr.ip = ctx->registers[reg]; + } + + reg = nft_rule_expr_get_u32(e, NFT_EXPR_NAT_REG_PROTO_MIN); + if (reg) + range.min_proto.all = ctx->registers[reg]; + + reg = nft_rule_expr_get_u32(e, NFT_EXPR_NAT_REG_PROTO_MAX); + if (reg) { + range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED; + range.max_proto.all = ctx->registers[reg]; + } + + nft_nat_dispatcher(nat_type, family, &range); +} 2) Then, this parser generates a structure that contains what most NAT extension need. The immediate parser stores the values in the ctx->registers, so the NAT will find what it needs there. The dispatcher is specific per instruction, so we can look up for the corresponding xt extension base on some specific tuple, in this case, the family and the nat type: +static void nft_nat_dispatcher(enum nft_nat_types nat_type, int fa + struct nf_nat_range *range) +{ + struct nft_xt_nat *xt_nat; + + list_for_each_entry(xt_nat, &nft_xt_nat_list, head) { + if (xt_nat->family == family && + xt_nat->nat_type == nat_type) { + xt_nat->parse(range); + return; + } + } +} This is currently a list, but we could implement it using a hashtable in the payload case. The xt_nat->parse function is defined in the corresponding xt extension. Anyway, I really think we have to start by converting nft to ipt command state in one single patch, as your patches 11-13 do, we need it for whatever approach we decide to follow. If you don't have time to make that rebase, I'll try to find some spare time to work on it. Let me know. Regards. -- 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, >> Could you recheck in detail patches 3/16 and 4/16? >> If there is no flaws in the translation engine, we might just go for >> it at it is. > I'm not convinced that this approach is the way to go. In the payload > case, the number of instruction tuples will explode, and you will have > to iterate many times to find the corresponding parser. There are many ways to reduce drastically such lookup process, current implementation is just here to prove it works. I have actually some code for that, by extending the instruction declaration with extra infos, it kills looping. Not finished though. (have also a solution for not pre-loading all extensions, btw) Actually NAT is probably the worst example. I should have implemented 2 or 3 totally different other extensions. Because nat exists as its own expression in nftables, so indeed it's really easy to pick it up in an expression list. if (expr == "nat") parse_nat(); But it's not going to be that simple with complex set of different expressions representing one extension. Of course it's can still work with programmatic way but it's going to be really annoying in maintenance, entangled code, bug prone etc... Since every time you will support a new extension, code will need to change. With my approach: no code change needed anymore, just declaring the right instruction which is very trivial. > Anyway, I really think we have to start by converting nft to ipt > command state in one single patch, as your patches 11-13 do, we need > it for whatever approach we decide to follow. If you don't have time > to make that rebase, I'll try to find some spare time to work on it. > Let me know. I can find time. 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
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c index 038d04f..02b52df 100644 --- a/iptables/nft-ipv4.c +++ b/iptables/nft-ipv4.c @@ -301,15 +301,10 @@ static void nft_ipv4_print_firewall(struct nft_rule *r, unsigned int num, unsigned int format) { struct iptables_command_state cs = {}; - const char *targname = NULL; - const void *targinfo = NULL; - size_t target_len = 0; nft_rule_to_iptables_command_state(r, &cs); - targname = nft_parse_target(r, &targinfo, &target_len); - - print_firewall_details(&cs, targname, cs.fw.ip.flags, + print_firewall_details(&cs, cs.jumpto, cs.fw.ip.flags, cs.fw.ip.invflags, cs.fw.ip.proto, cs.fw.ip.iniface, cs.fw.ip.outiface, num, format); @@ -324,11 +319,7 @@ static void nft_ipv4_print_firewall(struct nft_rule *r, unsigned int num, printf("[goto] "); #endif - if (print_matches(r, format) != 0) - return; - - if (print_target(targname, targinfo, target_len, format) != 0) - return; + print_matches_and_target(&cs, format); if (!(format & FMT_NONEWLINE)) fputc('\n', stdout); diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c index 5c79912..15c37f6 100644 --- a/iptables/nft-ipv6.c +++ b/iptables/nft-ipv6.c @@ -199,15 +199,10 @@ static void nft_ipv6_print_firewall(struct nft_rule *r, unsigned int num, unsigned int format) { struct iptables_command_state cs = {}; - const char *targname = NULL; - const void *targinfo = NULL; - size_t target_len = 0; nft_rule_to_iptables_command_state(r, &cs); - targname = nft_parse_target(r, &targinfo, &target_len); - - print_firewall_details(&cs, targname, cs.fw6.ipv6.flags, + print_firewall_details(&cs, cs.jumpto, cs.fw6.ipv6.flags, cs.fw6.ipv6.invflags, cs.fw6.ipv6.proto, cs.fw6.ipv6.iniface, cs.fw6.ipv6.outiface, num, format); @@ -222,11 +217,7 @@ static void nft_ipv6_print_firewall(struct nft_rule *r, unsigned int num, printf("[goto] "); #endif - if (print_matches(r, format) != 0) - return; - - if (print_target(targname, targinfo, target_len, format) != 0) - return; + print_matches_and_target(&cs, format); if (!(format & FMT_NONEWLINE)) fputc('\n', stdout); diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c index 8bef696..4eba145 100644 --- a/iptables/nft-shared.c +++ b/iptables/nft-shared.c @@ -284,59 +284,6 @@ void parse_meta(struct nft_rule_expr *e, uint8_t key, char *iniface, } } -const char *nft_parse_target(struct nft_rule *r, const void **targinfo, - size_t *target_len) -{ - struct nft_rule_expr_iter *iter; - struct nft_rule_expr *expr; - const char *targname = NULL; - - iter = nft_rule_expr_iter_create(r); - if (iter == NULL) - return NULL; - - expr = nft_rule_expr_iter_next(iter); - while (expr != NULL) { - const char *name = - nft_rule_expr_get_str(expr, NFT_RULE_EXPR_ATTR_NAME); - - if (strcmp(name, "target") == 0) { - targname = nft_rule_expr_get_str(expr, - NFT_EXPR_TG_NAME); - *targinfo = nft_rule_expr_get(expr, NFT_EXPR_TG_INFO, - target_len); - break; - } else if (strcmp(name, "immediate") == 0) { - uint32_t verdict = - nft_rule_expr_get_u32(expr, NFT_EXPR_IMM_VERDICT); - - switch(verdict) { - case NF_ACCEPT: - targname = "ACCEPT"; - break; - case NF_DROP: - targname = "DROP"; - break; - case NFT_RETURN: - targname = "RETURN"; - break; - case NFT_GOTO: - targname = nft_rule_expr_get_str(expr, - NFT_EXPR_IMM_CHAIN); - break; - case NFT_JUMP: - targname = nft_rule_expr_get_str(expr, - NFT_EXPR_IMM_CHAIN); - break; - } - } - expr = nft_rule_expr_iter_next(iter); - } - nft_rule_expr_iter_destroy(iter); - - return targname; -} - void print_proto(uint16_t proto, int invert) { const struct protoent *pent = getprotobynumber(proto); @@ -524,87 +471,6 @@ void nft_rule_to_iptables_command_state(struct nft_rule *r, i2cs.cs->jumpto = ""; } -static void -print_match(struct nft_rule_expr *expr, int numeric) -{ - size_t len; - const char *match_name = nft_rule_expr_get_str(expr, NFT_EXPR_MT_NAME); - const void *match_info = nft_rule_expr_get(expr, NFT_EXPR_MT_INFO, &len); - const struct xtables_match *match = - xtables_find_match(match_name, XTF_TRY_LOAD, NULL); - struct xt_entry_match *m = - calloc(1, sizeof(struct xt_entry_match) + len); - - /* emulate struct xt_entry_match since ->print needs it */ - memcpy((void *)&m->data, match_info, len); - - if (match) { - if (match->print) - /* FIXME missing first parameter */ - match->print(NULL, m, numeric); - else - printf("%s ", match_name); - } else { - if (match_name[0]) - printf("UNKNOWN match `%s' ", match_name); - } - - free(m); -} - -int print_matches(struct nft_rule *r, int format) -{ - struct nft_rule_expr_iter *iter; - struct nft_rule_expr *expr; - - iter = nft_rule_expr_iter_create(r); - if (iter == NULL) - return -ENOMEM; - - expr = nft_rule_expr_iter_next(iter); - while (expr != NULL) { - const char *name = - nft_rule_expr_get_str(expr, NFT_RULE_EXPR_ATTR_NAME); - - if (strcmp(name, "match") == 0) - print_match(expr, format & FMT_NUMERIC); - - expr = nft_rule_expr_iter_next(iter); - } - nft_rule_expr_iter_destroy(iter); - - return 0; -} - -int print_target(const char *targname, const void *targinfo, - size_t target_len, int format) -{ - struct xtables_target *target; - struct xt_entry_target *t; - - if (targname == NULL) - return 0; - - t = calloc(1, sizeof(struct xt_entry_target) + target_len); - if (t == NULL) - return -ENOMEM; - - /* emulate struct xt_entry_target since ->print needs it */ - memcpy((void *)&t->data, targinfo, target_len); - - target = xtables_find_target(targname, XTF_TRY_LOAD); - if (target) { - if (target->print) - /* FIXME missing first parameter */ - target->print(NULL, t, format & FMT_NUMERIC); - } else if (target_len > 0) - printf("[%ld bytes of unknown target data] ", target_len); - - free(t); - - return 0; -} - void print_num(uint64_t number, unsigned int format) { if (format & FMT_KILOMEGAGIGA) { @@ -694,6 +560,24 @@ void print_firewall_details(const struct iptables_command_state *cs, } } +void print_matches_and_target(struct iptables_command_state *cs, + unsigned int format) +{ + struct xtables_rule_match *matchp; + + for (matchp = cs->matches; matchp; matchp = matchp->next) { + if (matchp->match->print != NULL) + matchp->match->print(NULL, matchp->match->m, + format & FMT_NUMERIC); + } + + if (cs->target != NULL) { + if (cs->target->print != NULL) + cs->target->print(NULL, cs->target->t, + format & FMT_NUMERIC); + } +} + static enum nft_instruction nft_ipt_counters_instructions[] = { NFT_INSTRUCTION_COUNTER, NFT_INSTRUCTION_MAX, diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h index 176abed..20e49bd 100644 --- a/iptables/nft-shared.h +++ b/iptables/nft-shared.h @@ -84,8 +84,6 @@ bool is_same_interfaces(const char *a_iniface, const char *a_outiface, void parse_meta(struct nft_rule_expr *e, uint8_t key, char *iniface, unsigned char *iniface_mask, char *outiface, unsigned char *outiface_mask, uint8_t *invflags); -const char *nft_parse_target(struct nft_rule *r, const void **targinfo, - size_t *target_len); void print_proto(uint16_t proto, int invert); void get_expr_cmp_data(struct nft_rule_expr *e, void *data, size_t dlen, bool *inv); @@ -93,15 +91,14 @@ void get_cmp_data(struct nft_rule_expr_iter *iter, void *data, size_t dlen, bool *inv); void nft_rule_to_iptables_command_state(struct nft_rule *r, struct iptables_command_state *cs); -int print_matches(struct nft_rule *r, int format); -int print_target(const char *targname, const void *targinfo, - size_t target_len, int format); void print_num(uint64_t number, unsigned int format); void print_firewall_details(const struct iptables_command_state *cs, const char *targname, uint8_t flags, uint8_t invflags, uint8_t proto, const char *iniface, const char *outiface, unsigned int num, unsigned int format); +void print_matches_and_target(struct iptables_command_state *cs, + unsigned int format); int nft_initiate_translation_tree(void); struct nft_family_ops *nft_family_ops_lookup(int family);
Now that we parse properly, in one place and at once, the rule back into a command structure, it's now easier to print the rule from that command structure. Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com> --- iptables/nft-ipv4.c | 13 +---- iptables/nft-ipv6.c | 13 +---- iptables/nft-shared.c | 152 ++++++-------------------------------------------- iptables/nft-shared.h | 7 +-- 4 files changed, 24 insertions(+), 161 deletions(-)