[ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[Arturo Borrero Gonzalez]

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(-)

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"",

Re: [ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[Pablo Neira Ayuso]

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.

Re: [ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[Arturo Borrero Gonzalez]

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?
-- 
Arturo Borrero González
--
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: [ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[Pablo Neira Ayuso]

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.

Re: [ebtables-compat PATCH] ebtables-compat: fix ACCEPT printing by simplifying logic

[Arturo Borrero Gonzalez]

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.
-- 
Arturo Borrero González
--
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