| Message ID | 20160301204042.GA15382@sonyv |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
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 -- 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
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? -- 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 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. -- 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
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 :) -- 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 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? -- 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
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... -- 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 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
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
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
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
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
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? -- 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 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. -- 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
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)
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(+)