[PATCH v3] extensions: libxt_statistic: Add translation to nft

[PATCH v3] extensions: libxt_statistic: Add translation to nft

[Laura Garcia Liebana]

Add translation for random mode to nftables. The nth mode is not
supported yet.

Examples:

$ iptables-translate -A INPUT -m statistic --mode random --probability
0.1 -j ACCEPT
nft add rule ip filter INPUT meta random 0.10000000009 counter accept

$ iptables-translate -A INPUT -m statistic --mode random ! --probability
0.1 -j ACCEPT
nft add rule ip filter INPUT meta random != 0.10000000009 counter accept

The .xlate indirection returns 0 if the translation is not available.

Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
---
Changes in v2:
	- Return 0 if the translation is not supported, as Pablo suggested.
	- Include not supported modes in the commit message, as Shivani suggested.
Changes in v3:
	- Fix wrong email format.

 extensions/libxt_statistic.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/extensions/libxt_statistic.c b/extensions/libxt_statistic.c
index b6ae5f5..c771363 100644
--- a/extensions/libxt_statistic.c
+++ b/extensions/libxt_statistic.c
@@ -133,6 +133,22 @@ static void statistic_save(const void *ip, const struct xt_entry_match *match)
 	print_match(info, "--");
 }
 
+static int statistic_xlate(const struct xt_entry_match *match,
+			   struct xt_xlate *xl, int numeric)
+{
+	const struct xt_statistic_info *info = (void *)match->data;
+
+	if (info->mode == XT_STATISTIC_MODE_RANDOM) {
+		xt_xlate_add(xl, "meta random%s %.11f ",
+			     (info->flags & XT_STATISTIC_INVERT) ? " !=" : "",
+			     1.0 * info->u.random.probability / 0x80000000);
+	} else {
+		return 0;
+	}
+
+	return 1;
+}
+
 static struct xtables_match statistic_match = {
 	.family		= NFPROTO_UNSPEC,
 	.name		= "statistic",
@@ -145,6 +161,7 @@ static struct xtables_match statistic_match = {
 	.print		= statistic_print,
 	.save		= statistic_save,
 	.x6_options	= statistic_opts,
+	.xlate		= statistic_xlate,
 };
 
 void _init(void)
-- 
2.7.0

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Tue, Mar 01, 2016 at 09:40:45PM +0100, Laura Garcia Liebana wrote:
> Add translation for random mode to nftables. The nth mode is not
> supported yet.
> 
> Examples:
> 
> $ iptables-translate -A INPUT -m statistic --mode random --probability
> 0.1 -j ACCEPT
> nft add rule ip filter INPUT meta random 0.10000000009 counter accept

Is this translation correct?

I can see in 

static bool
statistic_mt(const struct sk_buff *skb, struct xt_action_param *par)
{
        const struct xt_statistic_info *info = par->matchinfo;
        bool ret = info->flags & XT_STATISTIC_INVERT;
        int nval, oval;

        switch (info->mode) {
        case XT_STATISTIC_MODE_RANDOM:
                if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)

--probability seems to check for "less than" the random value.

I think meta random 0.10000000009 will only match for the exact case.

So I suspect this is:

        meta random lt 0.10000000009

> $ iptables-translate -A INPUT -m statistic --mode random ! --probability
> 0.1 -j ACCEPT
> nft add rule ip filter INPUT meta random != 0.10000000009 counter accept

Then, the opposite has to be:

        meta random gte 0.10000000009

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Mar 01, 2016 at 09:40:45PM +0100, Laura Garcia Liebana wrote:
> > Add translation for random mode to nftables. The nth mode is not
> > supported yet.
> > 
> > Examples:
> > 
> > $ iptables-translate -A INPUT -m statistic --mode random --probability
> > 0.1 -j ACCEPT
> > nft add rule ip filter INPUT meta random 0.10000000009 counter accept
> 
> Is this translation correct?

Yes.

> I can see in 
> 
> static bool
> statistic_mt(const struct sk_buff *skb, struct xt_action_param *par)
> {
>         const struct xt_statistic_info *info = par->matchinfo;
>         bool ret = info->flags & XT_STATISTIC_INVERT;
>         int nval, oval;
> 
>         switch (info->mode) {
>         case XT_STATISTIC_MODE_RANDOM:
>                 if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)
> 
> --probability seems to check for "less than" the random value.

Yes.

> I think meta random 0.10000000009 will only match for the exact case.

No; I thought that 'nft ... meta random 0.5' should on average match half of
the time so the proposed nft prandom patch set makes LE the default op.

So meta random 0.1 is in fact 'meta random le 0.1' (and nft will display
it like this).

> > $ iptables-translate -A INPUT -m statistic --mode random ! --probability
> > 0.1 -j ACCEPT
> > nft add rule ip filter INPUT meta random != 0.10000000009 counter accept
> 
> Then, the opposite has to be:
> 
>         meta random gte 0.10000000009

Good point, this is not intuitive.

Currently if no operator is given and the type is TYPE_PROBABILITY then
we just use le instead of eq (just like we pick "&" in some cases).

But if user asks 'meta random ne 0.1' then the match propability is close
to 100%.

Do you think its enough to just document that you need to use le/ge etc.
for this?

Other option would be to rewrite NE to GE if rh value is a probability,
but I'm not sure if such 'helpful' logic isn't too likely to get in the
way.

Yet another option is to just disallow EQ and NE ops and throw an error.

Other suggestions?

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 02, 2016 at 01:10:33PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I think meta random 0.10000000009 will only match for the exact case.
> 
> No; I thought that 'nft ... meta random 0.5' should on average match half of
> the time so the proposed nft prandom patch set makes LE the default op.
> 
> So meta random 0.1 is in fact 'meta random le 0.1' (and nft will display
> it like this).
> 
> > > $ iptables-translate -A INPUT -m statistic --mode random ! --probability
> > > 0.1 -j ACCEPT
> > > nft add rule ip filter INPUT meta random != 0.10000000009 counter accept
> > 
> > Then, the opposite has to be:
> > 
> >         meta random gte 0.10000000009
> 
> Good point, this is not intuitive.
> 
> Currently if no operator is given and the type is TYPE_PROBABILITY then
> we just use le instead of eq (just like we pick "&" in some cases).
> 
> But if user asks 'meta random ne 0.1' then the match propability is close
> to 100%.
> 
> Do you think its enough to just document that you need to use le/ge etc.
> for this?
>
> Other option would be to rewrite NE to GE if rh value is a probability,
> but I'm not sure if such 'helpful' logic isn't too likely to get in the
> way.
> 
> Yet another option is to just disallow EQ and NE ops and throw an error.
> 
> Other suggestions?

I'm fine with the probability scaling, but I think we should keep this
consistent with other selectors, so I would use lt and gte instead
here.

We can potentially use ranges here too and other available operations
such as prefixes (although this one I don't know use case for this).

Thanks.

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:

[ nft meta random ]

> I'm fine with the probability scaling, but I think we should keep this
> consistent with other selectors, so I would use lt and gte instead
> here.
> 
> We can potentially use ranges here too and other available operations
> such as prefixes (although this one I don't know use case for this).

Ok, so just to clarify.  You want me to submit v2 of nft meta random
patch set that turns:

meta random value
into
meta random lt value

... and ...

meta random ne value
into
meta random ge value

Is that correct?

Sorry, I am a bit confused :)

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 02, 2016 at 01:37:38PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> [ nft meta random ]
> 
> > I'm fine with the probability scaling, but I think we should keep this
> > consistent with other selectors, so I would use lt and gte instead
> > here.
> > 
> > We can potentially use ranges here too and other available operations
> > such as prefixes (although this one I don't know use case for this).
> 
> Ok, so just to clarify.  You want me to submit v2 of nft meta random
> patch set that turns:
> 
> meta random value
> into
> meta random lt value
> 
> ... and ...
> 
> meta random ne value
> into
> meta random ge value
> 
> Is that correct?

I think so, so this becomes consistent with other selectors that we
have. Does this sound reasonable to you?

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Mar 02, 2016 at 01:37:38PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 
> > [ nft meta random ]
> > 
> > > I'm fine with the probability scaling, but I think we should keep this
> > > consistent with other selectors, so I would use lt and gte instead
> > > here.
> > > 
> > > We can potentially use ranges here too and other available operations
> > > such as prefixes (although this one I don't know use case for this).
> > 
> > Ok, so just to clarify.  You want me to submit v2 of nft meta random
> > patch set that turns:
> > 
> > meta random value
> > into
> > meta random lt value
> > 
> > ... and ...
> > 
> > meta random ne value
> > into
> > meta random ge value
> > 
> > Is that correct?
> 
> I think so, so this becomes consistent with other selectors that we
> have. Does this sound reasonable to you?

I tried to find an existing op that behaves this way, but did not find
any.

The first part (making meta random value translate to meta random le
value) seems fine, this is similar to e.g. tcp flags which uses '&' as
default op when user did not provide an operator.

But for a given operation, I could not find any place where we 'disobey'
the op and silently use something else.
So I disagree with second part -- i think meta random ne value
should be left as-is, i.e. NOT 'guess' that user wanted 'ge' instead.

I think users wanting 'negate meta random 0.9' should just
use 'meta random 0.1' 8-)

Regarding existing behaviour, this is what happens for
tcp flags:

"tcp flags syn":
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ]
  [ cmp neq reg 1 0x00000000 ]

-> i.e. we assume user wants bit-test, i.e. 'tcp flags & syn != 0'.

Some users instead expect this to behave like iptables
 --syn, i.e.  'match if syn is set and ack cleared'.

But I think its fine as-is, since 'tcp flags ack' does the sane
thing and matches when ACK flag is set, rather than *only* ACK
being set.

"tcp flags eq syn":
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ cmp eq reg 1 0x00000002 ]
"tcp flags ne syn":
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ cmp neq reg 1 0x00000002 ]

So we do what we're told for both eq and ne.
And as you can see, ne is NOT the inverse of the implicit
'tcp flags syn'.

To get the negation of 'tcp flags syn' user needs to ask for
"tcp flags & syn == 0":
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000000 ]

... and I think nft behaviour is consistent in this regard:

We attempt to pick the most sane op if nothing was provided
(eq in most cases, bit-test for some others) and otherwise
do what we're told.

Could do something like this:

@@ -1239,6 +1239,12 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 
                /* fall through */
        case OP_NEQ:
+               if (rel->right->dtype->type == TYPE_PROBABILITY)
+                       return expr_binary_error(ctx->msgs,right, left,
+                                                "Relational expression (%s) is undefined "
+                                                "for probability type",
+                                                expr_op_symbols[rel->op]);
+               /* fallthrough */
        case OP_FLAGCMP:

... but that could also prevent someone from doing something smart, so
I'm reluctant to disallow this just because this doesn't do what some people expect
at first glance...

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Jan Engelhardt]

On Wednesday 2016-03-02 13:10, Florian Westphal wrote:
>>         case XT_STATISTIC_MODE_RANDOM:
>>                 if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)
>> 
>> --probability seems to check for "less than" the random value.
>
>Yes. [...] 
>Other suggestions?

"--probability" is meant to represent saying "with a probability
of p=10%, ...". This does not mandate any particular operator.

The use of the LE operator seems more of an implementation detail for
use with discrete approaches (such as prandom_u32 and counting à la
Nth), and therefore should not be exposed by nft. Think of asking a
hypothetical hardware device which answers the probability question.

int mtinit(prob p) { setup_hw(p); }
bool match() { return hw_says(); }



Furthermore, it surprises me that iptables even supports
! --probability, because you can just express it as 1-p
instead.

"32% of people voted for Party 1, not 32% for Party 2"

Nobody does that. Instead,

"32% of people voted for Party 1, 68% (or: the rest) for Party 2"

which is probably also why I have never seen ! --p in the
wild, because anyone could just specify 1-p instead.
--
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: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Jan Engelhardt <jengelh@inai.de> wrote:
> On Wednesday 2016-03-02 13:10, Florian Westphal wrote:
> >>         case XT_STATISTIC_MODE_RANDOM:
> >>                 if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)
> >> 
> >> --probability seems to check for "less than" the random value.
> >
> >Yes. [...] 
> >Other suggestions?
> 
> "--probability" is meant to represent saying "with a probability
> of p=10%, ...". This does not mandate any particular operator.

Right, that was my reasoning for making meta random 0.1 behave
like 'match with a probabiliy of 10%'.

> Furthermore, it surprises me that iptables even supports
> ! --probability, because you can just express it as 1-p
> instead.

Yes.

So my suggestion is this:

for nft v2 of meta random support:

- keep the 'implicit LE op' behaviour so that
meta random 0.1 means '10% probability of matching'.
- change display to hide the LE detail from the user, i.e.
don't show 'meta random le 0.1' but 'meta random 0.1'.
[ I agree with Jan, its detail, users can still see this
with debug output on ].

Don't change anything else, i.e.

meta random == 0.1 will match with a probability of 1 in 0xfffffff
on average.  It does what you asked it to do ;)

For the translation patch, if ! is given, translate it to the inverse
as per Jans instruction, e.g.

--probability ! 0.1 is translated to

meta random 0.9

If there are no further comments, I will send a v2 for nft meta random
side soon.
--
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: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Jan Engelhardt]

On Wednesday 2016-03-02 15:50, Florian Westphal wrote:
>> 
>> "--probability" is meant to represent saying "with a probability
>> of p=10%, ...". This does not mandate any particular operator.
>
>So my suggestion is this:
>
>for nft v2 of meta random support:
>
>- keep the 'implicit LE op' behaviour so that
>meta random 0.1 means '10% probability of matching'.
>- change display to hide the LE detail from the user, i.e.
>don't show 'meta random le 0.1' but 'meta random 0.1'.
>[ I agree with Jan, its detail, users can still see this
>with debug output on ].

What I implied is that the operator ought to completely disappear,
also from the netlink exchange. Let the random module take
just p at the user-kernel boundary, like xt_statistic.c did.
--
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: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Jan Engelhardt <jengelh@inai.de> wrote:
> 
> On Wednesday 2016-03-02 15:50, Florian Westphal wrote:
> >> 
> >> "--probability" is meant to represent saying "with a probability
> >> of p=10%, ...". This does not mandate any particular operator.
> >
> >So my suggestion is this:
> >
> >for nft v2 of meta random support:
> >
> >- keep the 'implicit LE op' behaviour so that
> >meta random 0.1 means '10% probability of matching'.
> >- change display to hide the LE detail from the user, i.e.
> >don't show 'meta random le 0.1' but 'meta random 0.1'.
> >[ I agree with Jan, its detail, users can still see this
> >with debug output on ].
> 
> What I implied is that the operator ought to completely disappear,
> also from the netlink exchange. Let the random module take
> just p at the user-kernel boundary, like xt_statistic.c did.

This is what I want to avoid.

Right now meta random is 6 lines of kernel code;
It just fills a 32bit register with prandom_u32 result.
Everything else can be modeled with the nf_tables engine.

And I think thats the right approach, adding an nft_random
expression seems overkill.
--
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: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 02, 2016 at 03:50:16PM +0100, Florian Westphal wrote:
> Jan Engelhardt <jengelh@inai.de> wrote:
> > On Wednesday 2016-03-02 13:10, Florian Westphal wrote:
> > >>         case XT_STATISTIC_MODE_RANDOM:
> > >>                 if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)
> > >> 
> > >> --probability seems to check for "less than" the random value.
> > >
> > >Yes. [...] 
> > >Other suggestions?
> > 
> > "--probability" is meant to represent saying "with a probability
> > of p=10%, ...". This does not mandate any particular operator.
> 
> Right, that was my reasoning for making meta random 0.1 behave
> like 'match with a probabiliy of 10%'.
> 
> > Furthermore, it surprises me that iptables even supports
> > ! --probability, because you can just express it as 1-p
> > instead.
> 
> Yes.
> 
> So my suggestion is this:
> 
> for nft v2 of meta random support:
> 
> - keep the 'implicit LE op' behaviour so that
> meta random 0.1 means '10% probability of matching'.
> - change display to hide the LE detail from the user, i.e.
> don't show 'meta random le 0.1' but 'meta random 0.1'.
> [ I agree with Jan, its detail, users can still see this
> with debug output on ].
> 
> Don't change anything else, i.e.
> 
> meta random == 0.1 will match with a probability of 1 in 0xfffffff
> on average.  It does what you asked it to do ;)
> 
> For the translation patch, if ! is given, translate it to the inverse
> as per Jans instruction, e.g.
> 
> --probability ! 0.1 is translated to
> 
> meta random 0.9
> 
> If there are no further comments, I will send a v2 for nft meta random
> side soon.

In all this thread you talk all the time on probability semantics,
however the selector name is 'random'.

Why don't you rename this to 'meta probability' instead?

No changes in the semantics then, just use:

        meta probability 0.1

and when expressing the opposite:

        meta probability 0.9
--
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: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 02, 2016 at 03:50:16PM +0100, Florian Westphal wrote:
> Jan Engelhardt <jengelh@inai.de> wrote:
> > On Wednesday 2016-03-02 13:10, Florian Westphal wrote:
> > >>         case XT_STATISTIC_MODE_RANDOM:
> > >>                 if ((prandom_u32() & 0x7FFFFFFF) < info->u.random.probability)
> > >> 
> > >> --probability seems to check for "less than" the random value.
> > >
> > >Yes. [...] 
> > >Other suggestions?
> > 
> > "--probability" is meant to represent saying "with a probability
> > of�p=10%, ...". This does not mandate any particular operator.
> 
> Right, that was my reasoning for making meta random 0.1 behave
> like 'match with a probabiliy of 10%'.
> 
> > Furthermore, it surprises me that iptables even supports
> > !�--probability, because you can just express it as 1-p
> > instead.
> 
> Yes.
> 
> So my suggestion is this:
> 
> for nft v2 of meta random support:
> 
> - keep the 'implicit LE op' behaviour so that
> meta random 0.1 means '10% probability of matching'.
> - change display to hide the LE detail from the user, i.e.
> don't show 'meta random le 0.1' but 'meta random 0.1'.
> [ I agree with Jan, its detail, users can still see this
> with debug output on ].
> 
> Don't change anything else, i.e.
> 
> meta random == 0.1 will match with a probability of 1 in 0xfffffff
> on average.  It does what you asked it to do ;)
> 
> For the translation patch, if ! is given, translate it to the inverse
> as per Jans instruction, e.g.
> 
> --probability ! 0.1 is translated to
> 
> meta random 0.9
> 
> If there are no further comments, I will send a v2 for nft meta random
> side soon.

In all this thread you talk all the time on probability semantics,
however the selector name is 'random'.

Why don't you rename this to 'meta probability' instead?

No changes in the semantics then, just use:

        meta probability 0.1

and when expressing the opposite:

        meta probability 0.9

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> In all this thread you talk all the time on probability semantics,
> however the selector name is 'random'.
> 
> Why don't you rename this to 'meta probability' instead?
> 
> No changes in the semantics then, just use:
> 
>         meta probability 0.1
> 
> and when expressing the opposite:
> 
>         meta probability 0.9

I have no preferences one way or another, i'd
be fine with using probability for this.

In future you might want to allow something like

nft add rule filter input meta mark set meta random
or
nft add rule filter input queue num meta random

Perhaps we should use probability for now and later
alias is to random for these cases?

Re: [PATCH v3] extensions: libxt_statistic: Add translation to nft

[Pablo Neira Ayuso]

On Wed, Mar 02, 2016 at 04:29:46PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > In all this thread you talk all the time on probability semantics,
> > however the selector name is 'random'.
> > 
> > Why don't you rename this to 'meta probability' instead?
> > 
> > No changes in the semantics then, just use:
> > 
> >         meta probability 0.1
> > 
> > and when expressing the opposite:
> > 
> >         meta probability 0.9
> 
> I have no preferences one way or another, i'd
> be fine with using probability for this.
> 
> In future you might want to allow something like
> 
> nft add rule filter input meta mark set meta random

Right.

> or
> nft add rule filter input queue num meta random

Yes, this reminds me we have to fix nft_queue so it also accepts a
sreg as input. It's not very flexible and we cannot use maps with it.

> Perhaps we should use probability for now and later
> alias is to random for these cases?

+1 to using meta probability for this.