| Message ID | 20150113173610.3965.9202.stgit@nfdev.cica.es |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Tue, Jan 13, 2015 at 06:36:10PM +0100, Arturo Borrero Gonzalez wrote: > The commit bc543af ("ebtables-compat: fix segfault in rules w/o target") > doesn't handle all possible cases of target printing, and ACCEPT is left > behind. > > BTW, the logic of target (-j XXX) printing is a bit weird. This patch > simplifies it. > > I assume: > * cs->jumpto is only filled by nft_immediate. > * cs->target is only filled by nft_target. > > So we end with these cases: > * nft_immediate contains a 'standard' target (ACCEPT, DROP, CONTINUE, RETURN, chain) > Then cs->jumpto contains the target already. We have the rule. > * No standard target. If nft_target contains a target, try to load it. > * Neither nft_target nor nft_immediate exist. Then, assume CONTINUE. > > The printing path is then straight forward: either cs.jumpto or cs.target > contains the target. > > As there isn't support for target extensions yet, there is no way to test the > nft_target (cs.target) path. Not telling this is wrong, but I guess the resulting code to print the target has to converge to what we have in iptables-compat (see iptables/nft-ipv4.c). I mean, the handling should look similar. Could you revisit that and make sure that this and the existing code converge to the point? 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
On 15 January 2015 at 13:32, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Jan 13, 2015 at 06:36:10PM +0100, Arturo Borrero Gonzalez wrote: >> The commit bc543af ("ebtables-compat: fix segfault in rules w/o target") >> doesn't handle all possible cases of target printing, and ACCEPT is left >> behind. >> >> BTW, the logic of target (-j XXX) printing is a bit weird. This patch >> simplifies it. >> >> I assume: >> * cs->jumpto is only filled by nft_immediate. >> * cs->target is only filled by nft_target. >> >> So we end with these cases: >> * nft_immediate contains a 'standard' target (ACCEPT, DROP, CONTINUE, RETURN, chain) >> Then cs->jumpto contains the target already. We have the rule. >> * No standard target. If nft_target contains a target, try to load it. >> * Neither nft_target nor nft_immediate exist. Then, assume CONTINUE. >> >> The printing path is then straight forward: either cs.jumpto or cs.target >> contains the target. >> >> As there isn't support for target extensions yet, there is no way to test the >> nft_target (cs.target) path. > > Not telling this is wrong, but I guess the resulting code to print the > target has to converge to what we have in iptables-compat (see > iptables/nft-ipv4.c). I mean, the handling should look similar. Could > you revisit that and make sure that this and the existing code > converge to the point? Thanks. I could try to factorize code to a common function, something like: void nft_shared_rule_translate_target(char **jumpto, struct xtables_target **target) void nft_shared_print_target(const char *jumpto, const struct xtables_target *target) Do you like the idea?
On Thu, Jan 15, 2015 at 01:44:16PM +0100, Arturo Borrero Gonzalez wrote: > On 15 January 2015 at 13:32, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Jan 13, 2015 at 06:36:10PM +0100, Arturo Borrero Gonzalez wrote: > >> The commit bc543af ("ebtables-compat: fix segfault in rules w/o target") > >> doesn't handle all possible cases of target printing, and ACCEPT is left > >> behind. > >> > >> BTW, the logic of target (-j XXX) printing is a bit weird. This patch > >> simplifies it. > >> > >> I assume: > >> * cs->jumpto is only filled by nft_immediate. > >> * cs->target is only filled by nft_target. > >> > >> So we end with these cases: > >> * nft_immediate contains a 'standard' target (ACCEPT, DROP, CONTINUE, RETURN, chain) > >> Then cs->jumpto contains the target already. We have the rule. > >> * No standard target. If nft_target contains a target, try to load it. > >> * Neither nft_target nor nft_immediate exist. Then, assume CONTINUE. > >> > >> The printing path is then straight forward: either cs.jumpto or cs.target > >> contains the target. > >> > >> As there isn't support for target extensions yet, there is no way to test the > >> nft_target (cs.target) path. > > > > Not telling this is wrong, but I guess the resulting code to print the > > target has to converge to what we have in iptables-compat (see > > iptables/nft-ipv4.c). I mean, the handling should look similar. Could > > you revisit that and make sure that this and the existing code > > converge to the point? Thanks. > > I could try to factorize code to a common function, something like: > void nft_shared_rule_translate_target(char **jumpto, struct > xtables_target **target) > void nft_shared_print_target(const char *jumpto, const struct > xtables_target *target) > > Do you like the idea? Yes, the more we consolidate the less redundancy. Please, for the function names, I'd suggest a bit shorter ones: nft_set_target() and nft_print_target() I'd say. -- 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 15 January 2015 at 13:51, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Thu, Jan 15, 2015 at 01:44:16PM +0100, Arturo Borrero Gonzalez wrote: >> On 15 January 2015 at 13:32, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> > On Tue, Jan 13, 2015 at 06:36:10PM +0100, Arturo Borrero Gonzalez wrote: >> >> The commit bc543af ("ebtables-compat: fix segfault in rules w/o target") >> >> doesn't handle all possible cases of target printing, and ACCEPT is left >> >> behind. >> >> >> >> BTW, the logic of target (-j XXX) printing is a bit weird. This patch >> >> simplifies it. >> >> >> >> I assume: >> >> * cs->jumpto is only filled by nft_immediate. >> >> * cs->target is only filled by nft_target. >> >> >> >> So we end with these cases: >> >> * nft_immediate contains a 'standard' target (ACCEPT, DROP, CONTINUE, RETURN, chain) >> >> Then cs->jumpto contains the target already. We have the rule. >> >> * No standard target. If nft_target contains a target, try to load it. >> >> * Neither nft_target nor nft_immediate exist. Then, assume CONTINUE. >> >> >> >> The printing path is then straight forward: either cs.jumpto or cs.target >> >> contains the target. >> >> >> >> As there isn't support for target extensions yet, there is no way to test the >> >> nft_target (cs.target) path. >> > >> > Not telling this is wrong, but I guess the resulting code to print the >> > target has to converge to what we have in iptables-compat (see >> > iptables/nft-ipv4.c). I mean, the handling should look similar. Could >> > you revisit that and make sure that this and the existing code >> > converge to the point? Thanks. >> >> I could try to factorize code to a common function, something like: >> void nft_shared_rule_translate_target(char **jumpto, struct >> xtables_target **target) >> void nft_shared_print_target(const char *jumpto, const struct >> xtables_target *target) >> >> Do you like the idea? > > Yes, the more we consolidate the less redundancy. Please, for the > function names, I'd suggest a bit shorter ones: nft_set_target() and > nft_print_target() I'd say. Hi Pablo, reading the code again, I believe code paths we mentioned for iptables-compat and ebtables-compat are different enough for the code factorization to not worth it. However, I think that nft_rule_to_*tables_command_state() is more suitable for the factorization, but it is a different issue. let me know.
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c index fd9554e..9747405 100644 --- a/iptables/nft-bridge.c +++ b/iptables/nft-bridge.c @@ -337,12 +337,13 @@ void nft_rule_to_ebtables_command_state(struct nft_rule *r, nft_rule_expr_iter_destroy(iter); - if (cs->target != NULL) - cs->jumpto = cs->target->name; - else if (cs->jumpto != NULL) - cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD); + if (cs->jumpto != NULL) + return; + + if (cs->target != NULL && cs->target->name != NULL) + cs->target = xtables_find_target(cs->target->name, XTF_TRY_LOAD); else - cs->jumpto = ""; + cs->jumpto = "CONTINUE"; } static void print_iface(const char *iface) @@ -455,17 +456,11 @@ static void nft_bridge_print_firewall(struct nft_rule *r, unsigned int num, } printf("-j "); - if (cs.target != NULL) { - if (cs.target->print != NULL) { - cs.target->print(&cs.fw, cs.target->t, - format & FMT_NUMERIC); - } - } else { - if (strcmp(cs.jumpto, "") == 0) - printf("CONTINUE"); - else - printf("%s", cs.jumpto); - } + + if (cs.jumpto != NULL) + printf("%s", cs.jumpto); + else if (cs.target != NULL && cs.target->print != NULL) + cs.target->print(&cs.fw, cs.target->t, format & FMT_NUMERIC); if (!(format & FMT_NOCOUNTS)) printf(" , pcnt = %"PRIu64" -- bcnt = %"PRIu64"",
The commit bc543af ("ebtables-compat: fix segfault in rules w/o target") doesn't handle all possible cases of target printing, and ACCEPT is left behind. BTW, the logic of target (-j XXX) printing is a bit weird. This patch simplifies it. I assume: * cs->jumpto is only filled by nft_immediate. * cs->target is only filled by nft_target. So we end with these cases: * nft_immediate contains a 'standard' target (ACCEPT, DROP, CONTINUE, RETURN, chain) Then cs->jumpto contains the target already. We have the rule. * No standard target. If nft_target contains a target, try to load it. * Neither nft_target nor nft_immediate exist. Then, assume CONTINUE. The printing path is then straight forward: either cs.jumpto or cs.target contains the target. As there isn't support for target extensions yet, there is no way to test the nft_target (cs.target) path. Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> --- iptables/nft-bridge.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) -- 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