* [RFC ebtables-nft] unify ether type and meta protocol decoding @ 2022-11-30 11:37 Florian Westphal 2022-11-30 14:47 ` Phil Sutter 2022-12-20 20:44 ` [iptables RFC] ebtables: among: Embed meta protocol match into set lookup Phil Sutter 0 siblings, 2 replies; 5+ messages in thread From: Florian Westphal @ 2022-11-30 11:37 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal Handle "ether protocol" and "meta protcol" the same. Problem is that this breaks the test case *again*: I: [EXECUTING] iptables/tests/shell/testcases/ebtables/0006-flush_0 --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP ... because ebtables-nft will now render meta protocol as "-p IPv4". ebtables-legacy does not have any special handling for this. Solving this would need more internal annotations during decode, so we can suppress/ignore "meta protocol" once a "among-type" set is encountered. Any (other) suggestions? Signed-off-by: Florian Westphal <fw@strlen.de> --- iptables/nft-bridge.c | 74 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c index 50e90b22cf2f..4488ff172c2e 100644 --- a/iptables/nft-bridge.c +++ b/iptables/nft-bridge.c @@ -188,6 +188,64 @@ static int nft_bridge_add(struct nft_handle *h, return _add_action(r, cs); } +static bool nft_bridge_parse_ethproto(struct nft_xt_ctx *ctx, + struct nftnl_expr *e, + struct iptables_command_state *cs) +{ + struct ebt_entry *fw = &cs->eb; + bool already_seen; + uint16_t ethproto; + uint8_t op; + + already_seen = (fw->bitmask & EBT_NOPROTO) == 0; + + __get_cmp_data(e, ðproto, sizeof(ethproto), &op); + + switch (op) { + case NFT_CMP_EQ: + if (already_seen && fw->invflags & EBT_IPROTO) { + ctx->errmsg = "ethproto eq test contradicts previous"; + return false; + } + break; + case NFT_CMP_NEQ: + if (already_seen && (fw->invflags & EBT_IPROTO) == 0) { + ctx->errmsg = "ethproto ne test contradicts previous"; + return false; + } + fw->invflags |= EBT_IPROTO; + break; + case NFT_CMP_GTE: + if (already_seen && (fw->invflags & EBT_IPROTO) == 0) { + ctx->errmsg = "ethproto gte test contradicts previous"; + return false; + } + fw->invflags |= EBT_IPROTO; + /* fallthrough */ + case NFT_CMP_LT: + /* -p Length mode */ + if (ethproto == htons(0x0600)) + fw->bitmask |= EBT_802_3; + break; + default: + ctx->errmsg = "ethproto only supports eq/ne test"; + return false; + } + + if (already_seen) { + if (fw->ethproto != ethproto) { + ctx->errmsg = "ethproto ne test contradicts previous"; + return false; + } + } else if ((fw->bitmask & EBT_802_3) == 0) { + fw->ethproto = ethproto; + } + + fw->bitmask &= ~EBT_NOPROTO; + + return true; +} + static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx, const struct nft_xt_ctx_reg *reg, struct nftnl_expr *e, @@ -199,6 +257,7 @@ static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx, switch (reg->meta_dreg.key) { case NFT_META_PROTOCOL: + nft_bridge_parse_ethproto(ctx, e, cs); return; } @@ -241,8 +300,6 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx, { struct ebt_entry *fw = &cs->eb; unsigned char addr[ETH_ALEN]; - unsigned short int ethproto; - uint8_t op; bool inv; int i; @@ -275,17 +332,8 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx, fw->bitmask |= EBT_ISOURCE; break; case offsetof(struct ethhdr, h_proto): - __get_cmp_data(e, ðproto, sizeof(ethproto), &op); - if (ethproto == htons(0x0600)) { - fw->bitmask |= EBT_802_3; - inv = (op == NFT_CMP_GTE); - } else { - fw->ethproto = ethproto; - inv = (op == NFT_CMP_NEQ); - } - if (inv) - fw->invflags |= EBT_IPROTO; - fw->bitmask &= ~EBT_NOPROTO; + if (!nft_bridge_parse_ethproto(ctx, e, cs)) + return; break; } } -- 2.38.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC ebtables-nft] unify ether type and meta protocol decoding 2022-11-30 11:37 [RFC ebtables-nft] unify ether type and meta protocol decoding Florian Westphal @ 2022-11-30 14:47 ` Phil Sutter 2022-12-01 10:16 ` Florian Westphal 2022-12-20 20:44 ` [iptables RFC] ebtables: among: Embed meta protocol match into set lookup Phil Sutter 1 sibling, 1 reply; 5+ messages in thread From: Phil Sutter @ 2022-11-30 14:47 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote: > Handle "ether protocol" and "meta protcol" the same. > > Problem is that this breaks the test case *again*: > > I: [EXECUTING] iptables/tests/shell/testcases/ebtables/0006-flush_0 > --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > > ... because ebtables-nft will now render meta protocol as "-p IPv4". > > ebtables-legacy does not have any special handling for this. > > Solving this would need more internal annotations during decode, so > we can suppress/ignore "meta protocol" once a "among-type" set is > encountered. > > Any (other) suggestions? Since ebtables among does not support IPv6, match elimination should be pretty simple, no? Entirely untested: diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c index 08c93feeba2c9..0daebfd983127 100644 --- a/iptables/nft-bridge.c +++ b/iptables/nft-bridge.c @@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx, if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt)) xtables_error(OTHER_PROBLEM, "ebtables among pair parsing failed"); + + if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) && + ctx->cs.eb.ethproto == htons(0x0800)) + ctx->cs.eb.bitmask |= EBT_NOPROTO; } static void parse_watcher(void *object, struct ebt_match **match_list, Cheers, Phil ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC ebtables-nft] unify ether type and meta protocol decoding 2022-11-30 14:47 ` Phil Sutter @ 2022-12-01 10:16 ` Florian Westphal 2022-12-01 11:40 ` Phil Sutter 0 siblings, 1 reply; 5+ messages in thread From: Florian Westphal @ 2022-12-01 10:16 UTC (permalink / raw) To: Phil Sutter, Florian Westphal, netfilter-devel Phil Sutter <phil@nwl.cc> wrote: > On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote: > > Handle "ether protocol" and "meta protcol" the same. > > > > Problem is that this breaks the test case *again*: > > > > I: [EXECUTING] iptables/tests/shell/testcases/ebtables/0006-flush_0 > > --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > > --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > > +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > > +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > > > > ... because ebtables-nft will now render meta protocol as "-p IPv4". > > > > ebtables-legacy does not have any special handling for this. > > > > Solving this would need more internal annotations during decode, so > > we can suppress/ignore "meta protocol" once a "among-type" set is > > encountered. > > > > Any (other) suggestions? > > Since ebtables among does not support IPv6, match elimination should be > pretty simple, no? Entirely untested: > > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c > index 08c93feeba2c9..0daebfd983127 100644 > --- a/iptables/nft-bridge.c > +++ b/iptables/nft-bridge.c > @@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx, > if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt)) > xtables_error(OTHER_PROBLEM, > "ebtables among pair parsing failed"); > + > + if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) && > + ctx->cs.eb.ethproto == htons(0x0800)) > + ctx->cs.eb.bitmask |= EBT_NOPROTO; But that would munge ebtables-nft -p ipv4 .... ebtables-nft .... We don't know if "-p" was added explicitly or not. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC ebtables-nft] unify ether type and meta protocol decoding 2022-12-01 10:16 ` Florian Westphal @ 2022-12-01 11:40 ` Phil Sutter 0 siblings, 0 replies; 5+ messages in thread From: Phil Sutter @ 2022-12-01 11:40 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Thu, Dec 01, 2022 at 11:16:03AM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote: > > > Handle "ether protocol" and "meta protcol" the same. > > > > > > Problem is that this breaks the test case *again*: > > > > > > I: [EXECUTING] iptables/tests/shell/testcases/ebtables/0006-flush_0 > > > --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > > > --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > > > +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT > > > +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP > > > > > > ... because ebtables-nft will now render meta protocol as "-p IPv4". > > > > > > ebtables-legacy does not have any special handling for this. > > > > > > Solving this would need more internal annotations during decode, so > > > we can suppress/ignore "meta protocol" once a "among-type" set is > > > encountered. > > > > > > Any (other) suggestions? > > > > Since ebtables among does not support IPv6, match elimination should be > > pretty simple, no? Entirely untested: > > > > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c > > index 08c93feeba2c9..0daebfd983127 100644 > > --- a/iptables/nft-bridge.c > > +++ b/iptables/nft-bridge.c > > @@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx, > > if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt)) > > xtables_error(OTHER_PROBLEM, > > "ebtables among pair parsing failed"); > > + > > + if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) && > > + ctx->cs.eb.ethproto == htons(0x0800)) > > + ctx->cs.eb.bitmask |= EBT_NOPROTO; > > But that would munge > ebtables-nft -p ipv4 .... > ebtables-nft .... > > We don't know if "-p" was added explicitly or not. Ah, the infamous explicit vs. implicit problem. :( Looking at ebt_among.c in kernel, it seems packets which are neither IPv4 nor ARP are treated as non-matching. Since ebtables-nft doesn't support ARP with among anyway, can we assume people will not specify '-p ipv4' when using among? Cheers, Phil ^ permalink raw reply [flat|nested] 5+ messages in thread
* [iptables RFC] ebtables: among: Embed meta protocol match into set lookup 2022-11-30 11:37 [RFC ebtables-nft] unify ether type and meta protocol decoding Florian Westphal 2022-11-30 14:47 ` Phil Sutter @ 2022-12-20 20:44 ` Phil Sutter 1 sibling, 0 replies; 5+ messages in thread From: Phil Sutter @ 2022-12-20 20:44 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel This way the match will be consumed by among match parser and not confused with explicit '-p IPv4' match. Signed-off-by: Phil Sutter <phil@nwl.cc> --- This is a bit of a hack to avoid the implicit vs. explicit confusion but it works and is backwards compatible. WDYT? --- extensions/libebt_among.c | 1 + iptables/nft-bridge.c | 65 ++++++++++++++++++++++++--------------- iptables/nft-bridge.h | 1 + iptables/nft.c | 30 ++++++++++++------ 4 files changed, 62 insertions(+), 35 deletions(-) diff --git a/extensions/libebt_among.c b/extensions/libebt_among.c index a80fb80404ee1..49580ea6d3fc2 100644 --- a/extensions/libebt_among.c +++ b/extensions/libebt_among.c @@ -69,6 +69,7 @@ parse_nft_among_pair(char *buf, struct nft_among_pair *pair, bool have_ip) if (!inet_pton(AF_INET, sep + 1, &pair->in)) xtables_error(PARAMETER_PROBLEM, "Invalid IP address '%s'", sep + 1); + pair->proto = htons(ETH_P_IP); } ether = ether_aton(buf); if (!ether) diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c index e223d19765f90..ebfc5de50b236 100644 --- a/iptables/nft-bridge.c +++ b/iptables/nft-bridge.c @@ -335,17 +335,26 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx, const struct nft_xt_ctx_reg *reg; int val, val2 = -1; - reg = nft_xt_ctx_get_sreg(ctx, sreg); - if (!reg) - return -1; - - if (reg->type != NFT_XT_REG_PAYLOAD) { - ctx->errmsg = "lookup reg is not payload type"; + switch (key_len) { + case 6: /* ether */ + case 12: /* + ipv4addr */ + case 16: /* + meta protocol */ + break; + default: + ctx->errmsg = "unsupported lookup key length"; return -1; } - switch (key_len) { - case 12: /* ether + ipv4addr */ + if (key_len >= 6) { /* ether */ + reg = nft_xt_ctx_get_sreg(ctx, sreg); + if (!reg) + return -1; + + if (reg->type != NFT_XT_REG_PAYLOAD) { + ctx->errmsg = "lookup reg is not payload type"; + return -1; + } + val = lookup_check_ether_payload(reg->payload.base, reg->payload.offset, reg->payload.len); @@ -355,7 +364,8 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx, reg->payload.len); return -1; } - + } + if (key_len >= 12) { /* + ipv4addr */ sreg = nft_get_next_reg(sreg, ETH_ALEN); reg = nft_xt_ctx_get_sreg(ctx, sreg); @@ -381,23 +391,25 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx, DEBUGP("mismatching payload match offsets\n"); return -1; } - break; - case 6: /* ether */ - val = lookup_check_ether_payload(reg->payload.base, - reg->payload.offset, - reg->payload.len); - if (val < 0) { - DEBUGP("unknown payload base/offset/len %d/%d/%d\n", - reg->payload.base, reg->payload.offset, - reg->payload.len); + } + if (key_len == 16) { + sreg = nft_get_next_reg(sreg, sizeof(struct in_addr)); + reg = nft_xt_ctx_get_sreg(ctx, sreg); + if (!reg) { + ctx->errmsg = "next lookup register is invalid"; + return -1; + } + + if (reg->type != NFT_XT_REG_META_DREG) { + ctx->errmsg = "next lookup reg is not meta type"; return -1; } - break; - default: - ctx->errmsg = "unsupported lookup key length"; - return -1; - } + if (reg->meta_dreg.key != NFT_META_PROTOCOL) { + ctx->errmsg = "meta reg not protocol type"; + return -1; + } + } if (dst) *dst = (val == 1); if (ip) @@ -422,15 +434,18 @@ static int set_elems_to_among_pairs(struct nft_among_pair *pairs, while ((elem = nftnl_set_elems_iter_next(iter))) { data = nftnl_set_elem_get(elem, NFTNL_SET_ELEM_KEY, &datalen); + struct nft_among_pair pair = {}; + if (!data) { fprintf(stderr, "BUG: set elem without key\n"); goto err; } - if (datalen > sizeof(*pairs)) { + if (datalen > sizeof(pair)) { fprintf(stderr, "BUG: overlong set elem\n"); goto err; } - nft_among_insert_pair(pairs, &tmpcnt, data); + memcpy(&pair, data, datalen); + nft_among_insert_pair(pairs, &tmpcnt, &pair); } ret = 0; err: diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h index eb1b3928b6543..c87065ef48195 100644 --- a/iptables/nft-bridge.h +++ b/iptables/nft-bridge.h @@ -125,6 +125,7 @@ int ebt_command_default(struct iptables_command_state *cs); struct nft_among_pair { struct ether_addr ether; struct in_addr in __attribute__((aligned (4))); + uint16_t proto __attribute__((aligned (4))); }; struct nft_among_data { diff --git a/iptables/nft.c b/iptables/nft.c index 430888e864a5f..05422c91f3827 100644 --- a/iptables/nft.c +++ b/iptables/nft.c @@ -1139,6 +1139,7 @@ gen_lookup(uint32_t sreg, const char *set_name, uint32_t set_id, uint32_t flags) /* from nftables:include/datatype.h, enum datatypes */ #define NFT_DATATYPE_IPADDR 7 #define NFT_DATATYPE_ETHERADDR 9 +#define NFT_DATATYPE_ETHERTYPE 10 static int __add_nft_among(struct nft_handle *h, const char *table, struct nftnl_rule *r, struct nft_among_pair *pairs, @@ -1164,6 +1165,11 @@ static int __add_nft_among(struct nft_handle *h, const char *table, type = type << CONCAT_TYPE_BITS | NFT_DATATYPE_IPADDR; len += sizeof(struct in_addr) + NETLINK_ALIGN - 1; len &= ~(NETLINK_ALIGN - 1); + + type = type << CONCAT_TYPE_BITS | NFT_DATATYPE_ETHERTYPE; + len += sizeof(uint16_t) + NETLINK_ALIGN - 1; + len &= ~(NETLINK_ALIGN - 1); + flags = NFT_SET_INTERVAL | NFT_SET_CONCAT; } @@ -1173,7 +1179,11 @@ static int __add_nft_among(struct nft_handle *h, const char *table, set_id = nftnl_set_get_u32(s, NFTNL_SET_ID); if (ip) { - uint8_t field_len[2] = { ETH_ALEN, sizeof(struct in_addr) }; + uint8_t field_len[] = { + ETH_ALEN, + sizeof(struct in_addr), + sizeof(uint16_t), + }; nftnl_set_set_data(s, NFTNL_SET_DESC_CONCAT, field_len, sizeof(field_len)); @@ -1211,6 +1221,15 @@ static int __add_nft_among(struct nft_handle *h, const char *table, if (!e) return -ENOMEM; nftnl_rule_add_expr(r, e); + + /* FIXME: Fix add_meta() instead to accept a reg */ + reg = nft_get_next_reg(reg, sizeof(struct in_addr)); + e = nftnl_expr_alloc("meta"); + if (!e) + return -ENOMEM; + nftnl_expr_set_u32(e, NFTNL_EXPR_META_KEY, NFT_META_PROTOCOL); + nftnl_expr_set_u32(e, NFTNL_EXPR_META_DREG, reg); + nftnl_rule_add_expr(r, e); } reg = NFT_REG_1; @@ -1228,15 +1247,6 @@ static int add_nft_among(struct nft_handle *h, struct nft_among_data *data = (struct nft_among_data *)m->data; const char *table = nftnl_rule_get(r, NFTNL_RULE_TABLE); - if ((data->src.cnt && data->src.ip) || - (data->dst.cnt && data->dst.ip)) { - uint16_t eth_p_ip = htons(ETH_P_IP); - uint8_t reg; - - add_meta(h, r, NFT_META_PROTOCOL, ®); - add_cmp_ptr(r, NFT_CMP_EQ, ð_p_ip, 2, reg); - } - if (data->src.cnt) __add_nft_among(h, table, r, data->pairs, data->src.cnt, false, data->src.inv, data->src.ip); -- 2.38.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-20 20:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-30 11:37 [RFC ebtables-nft] unify ether type and meta protocol decoding Florian Westphal 2022-11-30 14:47 ` Phil Sutter 2022-12-01 10:16 ` Florian Westphal 2022-12-01 11:40 ` Phil Sutter 2022-12-20 20:44 ` [iptables RFC] ebtables: among: Embed meta protocol match into set lookup Phil Sutter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).