[PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Add translation for connlabel to nftables.
For examples:

  # iptables-translate -A INPUT -m connlabel --label bit40
  nft add rule ip filter INPUT ct label bit40 counter

  # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
  nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 V2: fix wrong translation when "!" is set, pointed by Florian Westphal.
 extensions/libxt_connlabel.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/extensions/libxt_connlabel.c b/extensions/libxt_connlabel.c
index 1f83095..96b9aec 100644
--- a/extensions/libxt_connlabel.c
+++ b/extensions/libxt_connlabel.c
@@ -118,6 +118,27 @@ connlabel_mt_save(const void *ip, const struct xt_entry_match *match)
 	connlabel_mt_print_op(info, "--");
 }
 
+static int
+connlabel_mt_xlate(const void *ip, const struct xt_entry_match *match,
+		   struct xt_xlate *xl, int numeric)
+{
+	const struct xt_connlabel_mtinfo *info = (const void *)match->data;
+	const char *name = connlabel_get_name(info->bit);
+
+	if (name == NULL)
+		return 0;
+
+	if (info->options & XT_CONNLABEL_OP_SET)
+		xt_xlate_add(xl, "ct label set %s ", name);
+
+	xt_xlate_add(xl, "ct label ");
+	if (info->options & XT_CONNLABEL_OP_INVERT)
+		xt_xlate_add(xl, "and %s != ", name);
+	xt_xlate_add(xl, "%s", name);
+
+	return 1;
+}
+
 static struct xtables_match connlabel_mt_reg = {
 	.family        = NFPROTO_UNSPEC,
 	.name          = "connlabel",
@@ -129,6 +150,7 @@ static struct xtables_match connlabel_mt_reg = {
 	.save          = connlabel_mt_save,
 	.x6_parse      = connlabel_mt_parse,
 	.x6_options    = connlabel_mt_opts,
+	.xlate	       = connlabel_mt_xlate,
 };
 
 void _init(void)
-- 
2.5.5

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Add translation for connlabel to nftables.
> For examples:
> 
>   # iptables-translate -A INPUT -m connlabel --label bit40
>   nft add rule ip filter INPUT ct label bit40 counter
> 
>   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
>   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> 
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>

Looks good, thanks!

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Pablo Neira Ayuso]

On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Add translation for connlabel to nftables.
> For examples:
> 
>   # iptables-translate -A INPUT -m connlabel --label bit40
>   nft add rule ip filter INPUT ct label bit40 counter
> 
>   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
>   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter

Applied, thanks.

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Pablo Neira Ayuso]

On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
>   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
>   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter

I think this logic is inverted, I mean:

nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
                             ---------------------------

test should happen before set.

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Pablo Neira Ayuso]

On Sat, Jul 16, 2016 at 04:51:30PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> 
> I think this logic is inverted, I mean:
> 
> nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
>                              ---------------------------
> 
> test should happen before set.

BTW, why not simply translate this to:

        nft add rule ip filter INPUT ct label set bit40 counter

nf_connlabels_replace() could be probably be updated from the kernel
to skip setting this again if already set.

This translation looks innecessarily large...

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> 
> I think this logic is inverted, I mean:
> 
> nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
>                              ---------------------------
> 
> test should happen before set.

xt_connlabel does:

if (info->options & XT_CONNLABEL_OP_SET)
	return (nf_connlabel_set(ct, info->bit) == 0) ^ invert;

So if info->bit is/got set (either it was already set or ct has connlabel
area where we could toggle the desired bit) then the match returns false
if invert was requested.

And to do that the "ct label set" operation needs to happen first.
To achive the same as the xt match in one op we'd need to alter the
"set" operation to also fill a dreg to perform a test; I don't think
its good idea.

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Jul 16, 2016 at 04:51:30PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> > >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> > >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > 
> > I think this logic is inverted, I mean:
> > 
> > nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
> >                              ---------------------------
> > 
> > test should happen before set.
> 
> BTW, why not simply translate this to:
> 
>         nft add rule ip filter INPUT ct label set bit40 counter

Its not the same as the bloated version.

The set operation will only ever fail in case the conntrack doesn't have a label
extension or is untracked/invalid, but if that is the case we get
different results:

nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter

-> counter Increments for every packet that lacks a conntrack, or the
conntrack extension

nft add rule ip filter INPUT ct label set bit40 counter

-> counter Increments for every packet (we don't set NFT_BREAK anywhere
in the setter).

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Pablo Neira Ayuso]

On Sat, Jul 16, 2016 at 08:12:51PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sat, Jul 16, 2016 at 04:51:30PM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> > > >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> > > >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > > 
> > > I think this logic is inverted, I mean:
> > > 
> > > nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
> > >                              ---------------------------
> > > 
> > > test should happen before set.
> > 
> > BTW, why not simply translate this to:
> > 
> >         nft add rule ip filter INPUT ct label set bit40 counter
> 
> Its not the same as the bloated version.
> 
> The set operation will only ever fail in case the conntrack doesn't have a label
>
> extension or is untracked/invalid, but if that is the case we get
> different results:
> 
> nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> 
> -> counter Increments for every packet that lacks a conntrack, or the
> conntrack extension
> 
> nft add rule ip filter INPUT ct label set bit40 counter
> 
> -> counter Increments for every packet (we don't set NFT_BREAK anywhere
> in the setter).

set operations are not expected to return anything at all, they must
always evaluate true.

This behaviour is deviating from what we have in other set operations,
this is clearly inconsistent.

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Jul 16, 2016 at 08:12:51PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Sat, Jul 16, 2016 at 04:51:30PM +0200, Pablo Neira Ayuso wrote:
> > > > On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> > > > >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> > > > >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > > > 
> > > > I think this logic is inverted, I mean:
> > > > 
> > > > nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
> > > >                              ---------------------------
> > > > 
> > > > test should happen before set.
> > > 
> > > BTW, why not simply translate this to:
> > > 
> > >         nft add rule ip filter INPUT ct label set bit40 counter
> > 
> > Its not the same as the bloated version.
> > 
> > The set operation will only ever fail in case the conntrack doesn't have a label
> >
> > extension or is untracked/invalid, but if that is the case we get
> > different results:
> > 
> > nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > 
> > -> counter Increments for every packet that lacks a conntrack, or the
> > conntrack extension
> > 
> > nft add rule ip filter INPUT ct label set bit40 counter
> > 
> > -> counter Increments for every packet (we don't set NFT_BREAK anywhere
> > in the setter).
> 
> set operations are not expected to return anything at all, they must
> always evaluate true.

Yes.

> This behaviour is deviating from what we have in other set operations,
> this is clearly inconsistent.

How so?

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Pablo Neira Ayuso]

On Sun, Jul 17, 2016 at 12:41:59PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sat, Jul 16, 2016 at 08:12:51PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Sat, Jul 16, 2016 at 04:51:30PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Sat, Jul 16, 2016 at 06:42:24PM +0800, Liping Zhang wrote:
> > > > > >   # iptables-translate -A INPUT -m connlabel ! --label bit40 --set
> > > > > >   nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > > > > 
> > > > > I think this logic is inverted, I mean:
> > > > > 
> > > > > nft add rule ip filter INPUT ct label and bit40 != bit40 ct label set bit40 counter
> > > > >                              ---------------------------
> > > > > 
> > > > > test should happen before set.
> > > > 
> > > > BTW, why not simply translate this to:
> > > > 
> > > >         nft add rule ip filter INPUT ct label set bit40 counter
> > > 
> > > Its not the same as the bloated version.
> > > 
> > > The set operation will only ever fail in case the conntrack doesn't have a label
> > >
> > > extension or is untracked/invalid, but if that is the case we get
> > > different results:
> > >
> > > nft add rule ip filter INPUT ct label set bit40 ct label and bit40 != bit40 counter
> > > 
> > > -> counter Increments for every packet that lacks a conntrack, or the
> > > conntrack extension
> > > 
> > > nft add rule ip filter INPUT ct label set bit40 counter
> > > 
> > > -> counter Increments for every packet (we don't set NFT_BREAK anywhere
> > > in the setter).
>
> > set operations are not expected to return anything at all, they must
> > always evaluate true.
> 
> Yes.
> 
> > This behaviour is deviating from what we have in other set operations,
> > this is clearly inconsistent.
> 
> How so?

So this is there just to cover the fail the ENOSPC when setting label?

This internal behaviour in xt connlabel seems confusing to me, this
rule:

        iptables -A INPUT -m connlabel ! --label bit40 --set

following the reading from left to right convention tells me:

        if not bit40 set, then set it.

But this is actually setting in first place inconditionally, then
checking this is not set, what is the use case for this?

Actually the kernel code first sets the bit, then checks if this is
unset for this. Note iptables-save displays this in that way as
output.

You can probably introduce in iptables something like:

        iptables -A INPUT -m connlabel --set-label bit40

that we can naturally map to nftables for newcomers using this, as
well as less expensive.

Re: [PATCH V2 iptables] extensions: libxt_connlabel: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > How so?
> 
> So this is there just to cover the fail the ENOSPC when setting label?

No label extension present or skb->nfct is untracked.
-m label --label bit40 will never match if the packet has no conntrack
attached.

"-m label --label bit40 --set" will behave the same in that case.

I would really prefer to expose this 1:1 in the translation
because it matches the behaviour.

Users that don't care about success can always just
"ct label set foo".

> This internal behaviour in xt connlabel seems confusing to me, this
> rule:
> 
>         iptables -A INPUT -m connlabel ! --label bit40 --set
> 
> following the reading from left to right convention tells me:
> 
>         if not bit40 set, then set it.

If not set, then *try* to set it:

if (ct == NULL || nf_ct_is_untracked(ct))
 return invert;

if (info->options & XT_CONNLABEL_OP_SET)
  return (nf_connlabel_set(ct, info->bit) == 0) ^ invert;

return connlabel_match(ct, info->bit) ^ invert;

> But this is actually setting in first place inconditionally, then
> checking this is not set, what is the use case for this?

The xt module doesn't have to recheck, if nf_connlabel_set returns 0
then the bit will be set.

> Actually the kernel code first sets the bit, then checks if this is
> unset for this. Note iptables-save displays this in that way as
> output.
> 
> You can probably introduce in iptables something like:
> 
>         iptables -A INPUT -m connlabel --set-label bit40

This is identical to

iptables -A INPUT -m connlabel --label bit40 --set

... unless you meant that this "--set-label bit40" should always return
true even if skb->nfct is NULL, but that seems wrong to me.

It would be more xtables-style to add
-j CONNLABEL --set-bit40

[ i.e. XT_CONTINUE regardless if we could set anything ]

But that doesn't make xtables any better and provides no benefit to
end users.