extensions: libxt_statistic: Add translation to nft

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.

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

index b6ae5f5..c771363 100644
@@ -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

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

Then, the opposite has to be:

        meta random gte 0.10000000009

Yes.

Yes.

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

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.

Pablo Neira Ayuso <[email protected]> wrote:

[ nft meta random ]

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

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...

"--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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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?

Right.

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.

+1 to using meta probability for this.