* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.