| Message ID | 20201002090334.29788-1-phil@nwl.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] nft: Optimize class-based IP prefix matches | expand |
On 2020-10-02 11:03, Phil Sutter wrote:
> +#define max(x, y) ((x) > (y) ? (x) : (y))
This is unused, right? not sure if on purpose.
On Fri, Oct 02, 2020 at 01:25:37PM +0200, Arturo Borrero Gonzalez wrote: > On 2020-10-02 11:03, Phil Sutter wrote: > > +#define max(x, y) ((x) > (y) ? (x) : (y)) > > This is unused, right? not sure if on purpose. Ah yes, I felt like adding it while being at it. Fine with you or should I drop it? Thanks for reviewing! Phil
On Fri, Oct 02, 2020 at 11:03:34AM +0200, Phil Sutter wrote: > Payload expression works on byte-boundaries, leverage this with suitable > prefix lengths. Interesing. But it kicks in the raw payload expression in nftables. # nft list ruleset table ip filter { chain INPUT { type filter hook input priority filter; policy accept; @nh,96,24 8323072 counter packets 0 bytes 0 } Would you send a patch for nftables too? There is already approximate offset matching in the tree, it should not be too hard to amend. Thanks.
On Tue, Oct 06, 2020 at 10:56:21AM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 02, 2020 at 11:03:34AM +0200, Phil Sutter wrote: > > Payload expression works on byte-boundaries, leverage this with suitable > > prefix lengths. > > Interesing. But it kicks in the raw payload expression in nftables. > > # nft list ruleset > table ip filter { > chain INPUT { > type filter hook input priority filter; policy accept; > @nh,96,24 8323072 counter packets 0 bytes 0 > } > > Would you send a patch for nftables too? There is already approximate > offset matching in the tree, it should not be too hard to amend. I had a quick look but it didn't seem trivial to me. It is in payload_expr_complete() where a template lookup happens based on expression offset and length which fails due to the unexpected length. Is this the right place to adjust or am I wrong? Strictly speaking, this is just a lack of feature in nftables and nothing breaks due to it. Do you still want to block the iptables change for it? Cheers, Phil
On Tue, Oct 06, 2020 at 11:37:44AM +0200, Phil Sutter wrote: > On Tue, Oct 06, 2020 at 10:56:21AM +0200, Pablo Neira Ayuso wrote: > > On Fri, Oct 02, 2020 at 11:03:34AM +0200, Phil Sutter wrote: > > > Payload expression works on byte-boundaries, leverage this with suitable > > > prefix lengths. > > > > Interesing. But it kicks in the raw payload expression in nftables. > > > > # nft list ruleset > > table ip filter { > > chain INPUT { > > type filter hook input priority filter; policy accept; > > @nh,96,24 8323072 counter packets 0 bytes 0 > > } > > > > Would you send a patch for nftables too? There is already approximate > > offset matching in the tree, it should not be too hard to amend. > > I had a quick look but it didn't seem trivial to me. It is in > payload_expr_complete() where a template lookup happens based on > expression offset and length which fails due to the unexpected length. > Is this the right place to adjust or am I wrong? > > Strictly speaking, this is just a lack of feature in nftables and > nothing breaks due to it. Do you still want to block the iptables change > for it? Not block. Just get things aligned. This is a bit of a step back in the integration between iptables-nft and nft IMO. I will have a look.
On Tue, Oct 06, 2020 at 11:41:21AM +0200, Pablo Neira Ayuso wrote: > On Tue, Oct 06, 2020 at 11:37:44AM +0200, Phil Sutter wrote: > > On Tue, Oct 06, 2020 at 10:56:21AM +0200, Pablo Neira Ayuso wrote: > > > On Fri, Oct 02, 2020 at 11:03:34AM +0200, Phil Sutter wrote: > > > > Payload expression works on byte-boundaries, leverage this with suitable > > > > prefix lengths. > > > > > > Interesing. But it kicks in the raw payload expression in nftables. > > > > > > # nft list ruleset > > > table ip filter { > > > chain INPUT { > > > type filter hook input priority filter; policy accept; > > > @nh,96,24 8323072 counter packets 0 bytes 0 > > > } > > > > > > Would you send a patch for nftables too? There is already approximate > > > offset matching in the tree, it should not be too hard to amend. > > > > I had a quick look but it didn't seem trivial to me. It is in > > payload_expr_complete() where a template lookup happens based on > > expression offset and length which fails due to the unexpected length. > > Is this the right place to adjust or am I wrong? > > > > Strictly speaking, this is just a lack of feature in nftables and > > nothing breaks due to it. Do you still want to block the iptables change > > for it? > > Not block. Just get things aligned. This is a bit of a step back in > the integration between iptables-nft and nft IMO. Well yes, it takes iptables-nft ahead in that regard. We should implement the same flexible payload size matching in nftables, too. > I will have a look. Thanks!
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c index afdecf9711e64..ce702041af0f4 100644 --- a/iptables/nft-ipv4.c +++ b/iptables/nft-ipv4.c @@ -199,7 +199,8 @@ static void nft_ipv4_parse_payload(struct nft_xt_ctx *ctx, parse_mask_ipv4(ctx, &cs->fw.ip.smsk); ctx->flags &= ~NFT_XT_CTX_BITWISE; } else { - cs->fw.ip.smsk.s_addr = 0xffffffff; + memset(&cs->fw.ip.smsk, 0xff, + min(ctx->payload.len, sizeof(struct in_addr))); } if (inv) @@ -212,7 +213,8 @@ static void nft_ipv4_parse_payload(struct nft_xt_ctx *ctx, parse_mask_ipv4(ctx, &cs->fw.ip.dmsk); ctx->flags &= ~NFT_XT_CTX_BITWISE; } else { - cs->fw.ip.dmsk.s_addr = 0xffffffff; + memset(&cs->fw.ip.dmsk, 0xff, + min(ctx->payload.len, sizeof(struct in_addr))); } if (inv) diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c index 4008b7eab4f2a..c877ec6d10887 100644 --- a/iptables/nft-ipv6.c +++ b/iptables/nft-ipv6.c @@ -146,7 +146,8 @@ static void nft_ipv6_parse_payload(struct nft_xt_ctx *ctx, parse_mask_ipv6(ctx, &cs->fw6.ipv6.smsk); ctx->flags &= ~NFT_XT_CTX_BITWISE; } else { - memset(&cs->fw6.ipv6.smsk, 0xff, sizeof(struct in6_addr)); + memset(&cs->fw6.ipv6.smsk, 0xff, + min(ctx->payload.len, sizeof(struct in6_addr))); } if (inv) @@ -159,7 +160,8 @@ static void nft_ipv6_parse_payload(struct nft_xt_ctx *ctx, parse_mask_ipv6(ctx, &cs->fw6.ipv6.dmsk); ctx->flags &= ~NFT_XT_CTX_BITWISE; } else { - memset(&cs->fw6.ipv6.dmsk, 0xff, sizeof(struct in6_addr)); + memset(&cs->fw6.ipv6.dmsk, 0xff, + min(ctx->payload.len, sizeof(struct in6_addr))); } if (inv) diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c index 7741d23befc5a..545e9c60fa015 100644 --- a/iptables/nft-shared.c +++ b/iptables/nft-shared.c @@ -166,16 +166,22 @@ void add_addr(struct nftnl_rule *r, int offset, void *data, void *mask, size_t len, uint32_t op) { const unsigned char *m = mask; + bool bitwise = false; int i; - add_payload(r, offset, len, NFT_PAYLOAD_NETWORK_HEADER); - for (i = 0; i < len; i++) { - if (m[i] != 0xff) + if (m[i] != 0xff) { + bitwise = m[i] != 0; break; + } } - if (i != len) + if (!bitwise) + len = i; + + add_payload(r, offset, len, NFT_PAYLOAD_NETWORK_HEADER); + + if (bitwise) add_bitwise(r, mask, len); add_cmp_ptr(r, op, data, len); diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h index 4440fd17bfeac..a52463342b30a 100644 --- a/iptables/nft-shared.h +++ b/iptables/nft-shared.h @@ -247,4 +247,8 @@ void xtables_restore_parse(struct nft_handle *h, const struct nft_xt_restore_parse *p); void nft_check_xt_legacy(int family, bool is_ipt_save); + +#define min(x, y) ((x) < (y) ? (x) : (y)) +#define max(x, y) ((x) > (y) ? (x) : (y)) + #endif
Payload expression works on byte-boundaries, leverage this with suitable prefix lengths. Signed-off-by: Phil Sutter <phil@nwl.cc> --- iptables/nft-ipv4.c | 6 ++++-- iptables/nft-ipv6.c | 6 ++++-- iptables/nft-shared.c | 14 ++++++++++---- iptables/nft-shared.h | 4 ++++ 4 files changed, 22 insertions(+), 8 deletions(-)