| Message ID | 1381846993-30093-1-git-send-email-fw@strlen.de |
|---|---|
| State | Deferred |
| Headers | show |
Hi Florian, On Tue, Oct 15, 2013 at 04:23:13PM +0200, Florian Westphal wrote: > Uses same syntax as iptables: itfname+. Good you're bringing up this issue, we've been discussing this for a while with recent Anand's patch. > The '+' suffix is not stored on the kernel side; this approach > is the same as the one used by iptables-nftables. Hm, it seems current iptables-nftables seems broken by: 73ea1cc nft: convert rule into a command state structure So let's have a look at the previous handling we had (which is the one I guess you're refering to): ifname_ptr = nft_rule_expr_get(e, NFT_EXPR_CMP_DATA, &len); memcpy(ifname, ifname_ptr, len); ifname[len] = '\0'; /* if this is zero, then assume this is a interface */ if (if_nametoindex(ifname) == 0) { ifname[len] = '+'; ifname[len+1] = '\0'; } That if_nametoindex handling was a bit of cheat: if the interface is gone after adding the rule, it will attach the '+', which is wrong. > +static void ifname_type_print(const struct expr *expr) > +{ > + unsigned int len = div_round_up(expr->len, BITS_PER_BYTE); > + char data[len]; > + > + mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len); > + printf("\"%.*s", len, data); > + if (len && data[len-1] != '\0') > + printf("+"); /* string without nul: interface wildcard match */ > + printf("\""); > +} Your assumption regarding trailing nul looks similar to what I can see in iptables, let's go that way in iptables-nftables. > i.e. xtables-save understands 'nft .. meta oifname foo+' and vice versa. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > Caveats: > - I am not convinced '+' is a good idea -- it is ambiguous since > 'foo+' is a legal interface name. I think we can remove the '+' in nft, so we match exactly what we pass for the ifname case, eg. iifname "eth". OK with this approach? > Maybe we should use 'foo/' (Linux forbids / in interface names) instead? > - added a new 'itfname' data type since I felt uncomfortable > with allowing 'non-nul-terminated' strings. > - removes a FIXME in netlink_delinearize. What was that about? :-} I don't remember the reason for that case, please try to dig it out from the history. 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: > On Tue, Oct 15, 2013 at 04:23:13PM +0200, Florian Westphal wrote: > > Uses same syntax as iptables: itfname+. > > Good you're bringing up this issue, we've been discussing this for a > while with recent Anand's patch. > > > The '+' suffix is not stored on the kernel side; this approach > > is the same as the one used by iptables-nftables. > > Hm, it seems current iptables-nftables seems broken by: > > 73ea1cc nft: convert rule into a command state structure I tested with latest ipt-nft (42531b3a6) -- admittingly, I did only test xt-save output, which adds '+' postfix in the no-trailing-nul case. > > Caveats: > > - I am not convinced '+' is a good idea -- it is ambiguous since > > 'foo+' is a legal interface name. > > I think we can remove the '+' in nft, so we match exactly what we > pass for the ifname case, eg. iifname "eth". Hm. "iifname eth1": Should it match eth1? Yes. But what about eth10, eth1.42, etc? I think we need an explicit way to resolve the ambiguity; relying on 'if_nametoinfex()' and just using index matching if we find an interface is not a good idea, it could fail too often in practice, or lead to unexpected results if rules are loaded before interfaces are brought up. > > - removes a FIXME in netlink_delinearize. What was that about? :-} > > I don't remember the reason for that case, please try to dig it out > from the history. Thanks! // FIXME if (left->len && left->dtype && left->dtype->type != TYPE_STRING ... is from Patricks initial commit. Lets see if Patrick remembers :) -- 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, Oct 16, 2013 at 01:00:43PM +0200, Florian Westphal wrote: > > > > - removes a FIXME in netlink_delinearize. What was that about? :-} > > > > I don't remember the reason for that case, please try to dig it out > > from the history. Thanks! > > // FIXME > if (left->len && left->dtype && left->dtype->type != TYPE_STRING > > ... is from Patricks initial commit. Lets see if Patrick remembers :) I already tried, but not off the top of my head. Let me check closer. Might take a few days though, currently quite busy. -- 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, Oct 16, 2013 at 01:00:43PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Oct 15, 2013 at 04:23:13PM +0200, Florian Westphal wrote: > > > Uses same syntax as iptables: itfname+. > > > > Good you're bringing up this issue, we've been discussing this for a > > while with recent Anand's patch. > > > > > The '+' suffix is not stored on the kernel side; this approach > > > is the same as the one used by iptables-nftables. > > > > Hm, it seems current iptables-nftables seems broken by: > > > > 73ea1cc nft: convert rule into a command state structure > > I tested with latest ipt-nft (42531b3a6) -- admittingly, I did only > test xt-save output, which adds '+' postfix in the no-trailing-nul case. > > > > Caveats: > > > - I am not convinced '+' is a good idea -- it is ambiguous since > > > 'foo+' is a legal interface name. > > > > I think we can remove the '+' in nft, so we match exactly what we > > pass for the ifname case, eg. iifname "eth". > > Hm. "iifname eth1": Should it match eth1? Yes. But what about eth10, > eth1.42, etc? I think we need an explicit way to resolve the ambiguity; I think "iffname eth1" should mean match "eth1\0". > relying on 'if_nametoinfex()' and just using index matching if we find > an interface is not a good idea, it could fail too often in practice, > or lead to unexpected results if rules are loaded before interfaces > are brought up. Agreed, if_nametoindex is not a good idea as I mentioned in my previous email. -- 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, Oct 16, 2013 at 02:58:39PM +0200, Pablo Neira Ayuso wrote: > On Wed, Oct 16, 2013 at 01:00:43PM +0200, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Tue, Oct 15, 2013 at 04:23:13PM +0200, Florian Westphal wrote: > > > > Uses same syntax as iptables: itfname+. > > > > > > Good you're bringing up this issue, we've been discussing this for a > > > while with recent Anand's patch. > > > > > > > The '+' suffix is not stored on the kernel side; this approach > > > > is the same as the one used by iptables-nftables. > > > > > > Hm, it seems current iptables-nftables seems broken by: > > > > > > 73ea1cc nft: convert rule into a command state structure > > > > I tested with latest ipt-nft (42531b3a6) -- admittingly, I did only > > test xt-save output, which adds '+' postfix in the no-trailing-nul case. > > > > > > Caveats: > > > > - I am not convinced '+' is a good idea -- it is ambiguous since > > > > 'foo+' is a legal interface name. > > > > > > I think we can remove the '+' in nft, so we match exactly what we > > > pass for the ifname case, eg. iifname "eth". > > > > Hm. "iifname eth1": Should it match eth1? Yes. But what about eth10, > > eth1.42, etc? I think we need an explicit way to resolve the ambiguity; > > I think "iffname eth1" should mean match "eth1\0". Oh I see, the ambiguity comes from nft syntax, then we do need some the wildcard character, yes. We can add "ifname-mask eth0", thus, ifname-mask eth1 means match "eth1", including eth1, eth1.0, etc. ifname eth1 means match "eth1\0". -- 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, Oct 16, 2013 at 6:31 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Oct 16, 2013 at 02:58:39PM +0200, Pablo Neira Ayuso wrote: >> On Wed, Oct 16, 2013 at 01:00:43PM +0200, Florian Westphal wrote: >> > Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> > > On Tue, Oct 15, 2013 at 04:23:13PM +0200, Florian Westphal wrote: >> > > > Uses same syntax as iptables: itfname+. >> > > >> > > Good you're bringing up this issue, we've been discussing this for a >> > > while with recent Anand's patch. >> > > >> > > > The '+' suffix is not stored on the kernel side; this approach >> > > > is the same as the one used by iptables-nftables. >> > > >> > > Hm, it seems current iptables-nftables seems broken by: >> > > >> > > 73ea1cc nft: convert rule into a command state structure >> > >> > I tested with latest ipt-nft (42531b3a6) -- admittingly, I did only >> > test xt-save output, which adds '+' postfix in the no-trailing-nul case. >> > >> > > > Caveats: >> > > > - I am not convinced '+' is a good idea -- it is ambiguous since >> > > > 'foo+' is a legal interface name. >> > > >> > > I think we can remove the '+' in nft, so we match exactly what we >> > > pass for the ifname case, eg. iifname "eth". >> > >> > Hm. "iifname eth1": Should it match eth1? Yes. But what about eth10, >> > eth1.42, etc? I think we need an explicit way to resolve the ambiguity; >> >> I think "iffname eth1" should mean match "eth1\0". > > Oh I see, the ambiguity comes from nft syntax, then we do need some > the wildcard character, yes. > > We can add "ifname-mask eth0", thus, > > ifname-mask eth1 means match "eth1", including eth1, eth1.0, etc. > ifname eth1 means match "eth1\0". iptables use the mask in kernel (ip_packet_match) to do the wildcard match. Currently ipt-nft does use the MASK for the rule validation and with patch submitted earlier , it should now help us to ADD/DELETE rules. The opinion here is to push the offset/mask validation on the nft_meta_eval in kernel ? -- 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 Tuesday 2013-10-15 16:23, Florian Westphal wrote: > Caveats: > - I am not convinced '+' is a good idea -- it is ambiguous since > 'foo+' is a legal interface name. > Maybe we should use 'foo/' (Linux forbids / in interface names) instead? While there are almost no limitations on the interface name, there is a limit to what characters are _useful_ to have in an interface name. Picking foo+ was a sensible decision back then, and continues to be, unless you want to go for the optically different 'foo*'. -- 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/include/datatype.h b/include/datatype.h index 239d5ea..9a6ae33 100644 --- a/include/datatype.h +++ b/include/datatype.h @@ -40,6 +40,7 @@ enum datatypes { TYPE_BITMASK, TYPE_INTEGER, TYPE_STRING, + TYPE_IFNAME, TYPE_LLADDR, TYPE_IPADDR, TYPE_IP6ADDR, @@ -128,7 +129,13 @@ extern void datatype_print(const struct expr *expr); static inline bool datatype_equal(const struct datatype *d1, const struct datatype *d2) { - return d1->type == d2->type; + if (d1->type == d2->type) + return true; + if (d1->basetype) + return datatype_equal(d1->basetype, d2); + if (d2->basetype) + return datatype_equal(d1, d2->basetype); + return false; } /** @@ -171,6 +178,7 @@ extern const struct datatype verdict_type; extern const struct datatype bitmask_type; extern const struct datatype integer_type; extern const struct datatype string_type; +extern const struct datatype ifname_type; extern const struct datatype lladdr_type; extern const struct datatype ipaddr_type; extern const struct datatype ip6addr_type; diff --git a/src/datatype.c b/src/datatype.c index 4c5a70f..e0dbe80 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -29,6 +29,7 @@ static const struct datatype *datatypes[TYPE_MAX + 1] = { [TYPE_BITMASK] = &bitmask_type, [TYPE_INTEGER] = &integer_type, [TYPE_STRING] = &string_type, + [TYPE_IFNAME] = &ifname_type, [TYPE_LLADDR] = &lladdr_type, [TYPE_IPADDR] = &ipaddr_type, [TYPE_IP6ADDR] = &ip6addr_type, @@ -280,6 +281,46 @@ const struct datatype string_type = { .parse = string_type_parse, }; +static void ifname_type_print(const struct expr *expr) +{ + unsigned int len = div_round_up(expr->len, BITS_PER_BYTE); + char data[len]; + + mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len); + printf("\"%.*s", len, data); + if (len && data[len-1] != '\0') + printf("+"); /* string without nul: interface wildcard match */ + printf("\""); +} + +static struct error_record *ifname_type_parse(const struct expr *sym, + struct expr **res) +{ + size_t len = strlen(sym->identifier); + + assert(len > 0); + + if (sym->identifier[len-1] != '+') + len++; /* exact match: count \0, too */ + else + len--; /* match "name*", ignore '+' */ + + *res = constant_expr_alloc(&sym->location, &string_type, + BYTEORDER_HOST_ENDIAN, + len * BITS_PER_BYTE, + sym->identifier); + return NULL; +} + +const struct datatype ifname_type = { + .type = TYPE_IFNAME, + .name = "ifname", + .desc = "ifname", + .print = ifname_type_print, + .parse = ifname_type_parse, + .basetype = &string_type, +}; + static void lladdr_type_print(const struct expr *expr) { unsigned int len = div_round_up(expr->len, BITS_PER_BYTE); diff --git a/src/evaluate.c b/src/evaluate.c index 94fee64..2625c2b 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -243,7 +243,6 @@ static int expr_evaluate_value(struct eval_ctx *ctx, struct expr **expr) return expr_error(ctx, *expr, "String exceeds maximum length of %u", ctx->ectx.len / BITS_PER_BYTE); - (*expr)->len = ctx->ectx.len; } break; default: diff --git a/src/meta.c b/src/meta.c index 9606a44..d8c6409 100644 --- a/src/meta.c +++ b/src/meta.c @@ -293,14 +293,14 @@ static const struct meta_template meta_templates[] = { 4 * 8, BYTEORDER_HOST_ENDIAN), [NFT_META_IIF] = META_TEMPLATE("iif", &ifindex_type, 4 * 8, BYTEORDER_HOST_ENDIAN), - [NFT_META_IIFNAME] = META_TEMPLATE("iifname", &string_type, + [NFT_META_IIFNAME] = META_TEMPLATE("iifname", &ifname_type, IFNAMSIZ * BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), [NFT_META_IIFTYPE] = META_TEMPLATE("iiftype", &arphrd_type, 2 * 8, BYTEORDER_HOST_ENDIAN), [NFT_META_OIF] = META_TEMPLATE("oif", &ifindex_type, 4 * 8, BYTEORDER_HOST_ENDIAN), - [NFT_META_OIFNAME] = META_TEMPLATE("oifname", &string_type, + [NFT_META_OIFNAME] = META_TEMPLATE("oifname", &ifname_type, IFNAMSIZ * BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), [NFT_META_OIFTYPE] = META_TEMPLATE("oiftype", &arphrd_type, diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c index d80fc78..adf34bf 100644 --- a/src/netlink_delinearize.c +++ b/src/netlink_delinearize.c @@ -117,6 +117,11 @@ static enum ops netlink_parse_cmp_op(const struct nft_rule_expr *nle) } } +static bool relational_expr_check_size(struct expr *expr) +{ + return expr->len && expr_basetype(expr)->type != TYPE_STRING; +} + static void netlink_parse_cmp(struct netlink_parse_ctx *ctx, const struct location *loc, const struct nft_rule_expr *nle) @@ -137,9 +142,7 @@ static void netlink_parse_cmp(struct netlink_parse_ctx *ctx, op = netlink_parse_cmp_op(nle); right = netlink_alloc_value(loc, &nld); - // FIXME - if (left->len && left->dtype && left->dtype->type != TYPE_STRING && - left->len != right->len) + if (relational_expr_check_size(left) && left->len != right->len) return netlink_error(ctx, loc, "Relational expression size mismatch"); diff --git a/src/scanner.l b/src/scanner.l index cee6aa6..fe93b8b 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -111,7 +111,7 @@ decstring {digit}+ hexstring 0[xX]{hexdigit}+ range ({decstring}?:{decstring}?) letter [a-zA-Z] -string ({letter})({letter}|{digit}|[/\-_\.])* +string ({letter})({letter}|{digit}|[/\-_\.\+])* quotedstring \"[^"]*\" comment #.*$ slash \/
Uses same syntax as iptables: itfname+. The '+' suffix is not stored on the kernel side; this approach is the same as the one used by iptables-nftables, i.e. xtables-save understands 'nft .. meta oifname foo+' and vice versa. Signed-off-by: Florian Westphal <fw@strlen.de> --- Caveats: - I am not convinced '+' is a good idea -- it is ambiguous since 'foo+' is a legal interface name. Maybe we should use 'foo/' (Linux forbids / in interface names) instead? - added a new 'itfname' data type since I felt uncomfortable with allowing 'non-nul-terminated' strings. - removes a FIXME in netlink_delinearize. What was that about? :-} include/datatype.h | 10 +++++++++- src/datatype.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/evaluate.c | 1 - src/meta.c | 4 ++-- src/netlink_delinearize.c | 9 ++++++--- src/scanner.l | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-)