* [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c
@ 2022-09-28 18:26 Vishwanath Pai
2022-09-28 18:26 ` [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel) Vishwanath Pai
2022-11-21 13:58 ` [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Pablo Neira Ayuso
0 siblings, 2 replies; 5+ messages in thread
From: Vishwanath Pai @ 2022-09-28 18:26 UTC (permalink / raw)
To: pablo, kadlec, fw; +Cc: Vishwanath Pai, johunt, netfilter-devel
This patch introduced a regression: commit 48596a8ddc46 ("netfilter:
ipset: Fix adding an IPv4 range containing more than 2^31 addresses")
The variable e.ip is passed to adtfn() function which finally adds the
ip address to the set. The patch above refactored the for loop and moved
e.ip = htonl(ip) to the end of the for loop.
What this means is that if the value of "ip" changes between the first
assignement of e.ip and the forloop, then e.ip is pointing to a
different ip address than "ip".
Test case:
$ ipset create jdtest_tmp hash:ip family inet hashsize 2048 maxelem 100000
$ ipset add jdtest_tmp 10.0.1.1/31
ipset v6.21.1: Element cannot be added to the set: it's already added
The value of ip gets updated inside the "else if (tb[IPSET_ATTR_CIDR])"
block but e.ip is still pointing to the old value.
Reviewed-by: Joshua Hunt <johunt@akamai.com>
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
---
Notes:
This is v2 of the patch, simplified based on feedback from Jozsef.
net/netfilter/ipset/ip_set_hash_ip.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
index dd30c03d5a23..75d556d71652 100644
--- a/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/net/netfilter/ipset/ip_set_hash_ip.c
@@ -151,18 +151,16 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
if (((u64)ip_to - ip + 1) >> (32 - h->netmask) > IPSET_MAX_RANGE)
return -ERANGE;
- if (retried) {
+ if (retried)
ip = ntohl(h->next.ip);
- e.ip = htonl(ip);
- }
for (; ip <= ip_to;) {
+ e.ip = htonl(ip);
ret = adtfn(set, &e, &ext, &ext, flags);
if (ret && !ip_set_eexist(ret, flags))
return ret;
ip += hosts;
- e.ip = htonl(ip);
- if (e.ip == 0)
+ if (ip == 0)
return 0;
ret = 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel)
2022-09-28 18:26 [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
@ 2022-09-28 18:26 ` Vishwanath Pai
2022-11-07 8:54 ` Jozsef Kadlecsik
2022-11-21 13:58 ` [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Pablo Neira Ayuso
1 sibling, 1 reply; 5+ messages in thread
From: Vishwanath Pai @ 2022-09-28 18:26 UTC (permalink / raw)
To: pablo, kadlec, fw; +Cc: Vishwanath Pai, johunt, netfilter-devel
Add a new parameter to complement the existing 'netmask' option. The
main difference between netmask and bitmask is that bitmask takes any
arbitrary ip address as input, it does not have to be a valid netmask.
The name of the new parameter is 'bitmask'. This lets us mask out
arbitrary bits in the ip address, for example:
ipset create set1 hash:ip bitmask 255.128.255.0
ipset create set2 hash:ip,port family inet6 bitmask ffff::ff80
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
Signed-off-by: Joshua Hunt <johunt@akamai.com>
---
Notes:
Changes in v2 based on code review comments:
* bitmask and netmask options are mutually exclusive now
* Added tests for the bitmask feature to all three set types
* IP_SET_HASH_WITH_BITMASK is a separate macro now
* netmask is now converted and stored as 'bitmask' and bitmask is
applied to the entries directly for better efficiency
* fixed comment for revision 6 in hash:ip
* removed netmask from hash:net,net, it has a cidr option already
include/linux/netfilter.h | 9 +++
include/uapi/linux/netfilter/ipset/ip_set.h | 2 +
net/netfilter/ipset/ip_set_hash_gen.h | 75 +++++++++++++++++----
net/netfilter/ipset/ip_set_hash_ip.c | 19 +++---
net/netfilter/ipset/ip_set_hash_ipport.c | 24 ++++++-
net/netfilter/ipset/ip_set_hash_netnet.c | 24 +++++--
6 files changed, 124 insertions(+), 29 deletions(-)
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index c2c6f332fb90..c7155eaebc84 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -56,6 +56,15 @@ static inline void nf_inet_addr_mask(const union nf_inet_addr *a1,
#endif
}
+static inline void nf_inet_addr_mask_inplace(union nf_inet_addr *a1,
+ const union nf_inet_addr *mask)
+{
+ a1->all[0] &= mask->all[0];
+ a1->all[1] &= mask->all[1];
+ a1->all[2] &= mask->all[2];
+ a1->all[3] &= mask->all[3];
+}
+
int netfilter_init(void);
struct sk_buff;
diff --git a/include/uapi/linux/netfilter/ipset/ip_set.h b/include/uapi/linux/netfilter/ipset/ip_set.h
index 6397d75899bc..1bfa765a2191 100644
--- a/include/uapi/linux/netfilter/ipset/ip_set.h
+++ b/include/uapi/linux/netfilter/ipset/ip_set.h
@@ -89,6 +89,7 @@ enum {
IPSET_ATTR_CADT_LINENO = IPSET_ATTR_LINENO, /* 9 */
IPSET_ATTR_MARK, /* 10 */
IPSET_ATTR_MARKMASK, /* 11 */
+ IPSET_ATTR_BITMASK, /* 12 */
/* Reserve empty slots */
IPSET_ATTR_CADT_MAX = 16,
/* Create-only specific attributes */
@@ -157,6 +158,7 @@ enum ipset_errno {
IPSET_ERR_COMMENT,
IPSET_ERR_INVALID_MARKMASK,
IPSET_ERR_SKBINFO,
+ IPSET_ERR_BITMASK_NETMASK_EXCL,
/* Type specific error codes */
IPSET_ERR_TYPE_SPECIFIC = 4352,
diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 6e391308431d..1f1b106a2cd8 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -182,6 +182,17 @@ htable_size(u8 hbits)
(SET_WITH_TIMEOUT(set) && \
ip_set_timeout_expired(ext_timeout(d, set)))
+#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
+static const union nf_inet_addr onesmask = {
+ .all[0] = 0xffffffff,
+ .all[1] = 0xffffffff,
+ .all[2] = 0xffffffff,
+ .all[3] = 0xffffffff
+};
+
+static const union nf_inet_addr zeromask;
+#endif
+
#endif /* _IP_SET_HASH_GEN_H */
#ifndef MTYPE
@@ -306,8 +317,9 @@ struct htype {
u32 markmask; /* markmask value for mark mask to store */
#endif
u8 bucketsize; /* max elements in an array block */
-#ifdef IP_SET_HASH_WITH_NETMASK
+#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
u8 netmask; /* netmask value for subnets to store */
+ union nf_inet_addr bitmask; /* stores bitmask */
#endif
struct list_head ad; /* Resize add|del backlist */
struct mtype_elem next; /* temporary storage for uadd */
@@ -482,8 +494,8 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
/* Resizing changes htable_bits, so we ignore it */
return x->maxelem == y->maxelem &&
a->timeout == b->timeout &&
-#ifdef IP_SET_HASH_WITH_NETMASK
- x->netmask == y->netmask &&
+#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
+ nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
#endif
#ifdef IP_SET_HASH_WITH_MARKMASK
x->markmask == y->markmask &&
@@ -1282,9 +1294,21 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
htonl(jhash_size(htable_bits))) ||
nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
goto nla_put_failure;
+#ifdef IP_SET_HASH_WITH_BITMASK
+ /* if netmask is set to anything other than HOST_MASK we know that the user supplied netmask
+ * and not bitmask. These two are mutually exclusive. */
+ if (h->netmask == HOST_MASK && !nf_inet_addr_cmp(&onesmask, &h->bitmask)) {
+ if (set->family == NFPROTO_IPV4) {
+ if (nla_put_ipaddr4(skb, IPSET_ATTR_BITMASK, h->bitmask.ip))
+ goto nla_put_failure;
+ } else if (set->family == NFPROTO_IPV6) {
+ if (nla_put_ipaddr6(skb, IPSET_ATTR_BITMASK, &h->bitmask.in6))
+ goto nla_put_failure;
+ }
+ }
+#endif
#ifdef IP_SET_HASH_WITH_NETMASK
- if (h->netmask != HOST_MASK &&
- nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
+ if (h->netmask != HOST_MASK && nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
goto nla_put_failure;
#endif
#ifdef IP_SET_HASH_WITH_MARKMASK
@@ -1447,8 +1471,10 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
u32 markmask;
#endif
u8 hbits;
-#ifdef IP_SET_HASH_WITH_NETMASK
- u8 netmask;
+#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
+ int ret __attribute__((unused)) = 0;
+ u8 netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
+ union nf_inet_addr bitmask = onesmask;
#endif
size_t hsize;
struct htype *h;
@@ -1486,13 +1512,37 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
#endif
#ifdef IP_SET_HASH_WITH_NETMASK
- netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
if (tb[IPSET_ATTR_NETMASK]) {
netmask = nla_get_u8(tb[IPSET_ATTR_NETMASK]);
-
if ((set->family == NFPROTO_IPV4 && netmask > 32) ||
- (set->family == NFPROTO_IPV6 && netmask > 128) ||
- netmask == 0)
+ (set->family == NFPROTO_IPV6 && netmask > 128))
+ return -IPSET_ERR_INVALID_NETMASK;
+
+ /* we convert netmask to bitmask and store it */
+ if (set->family == NFPROTO_IPV4)
+ bitmask.ip = ip_set_netmask(netmask);
+ else
+ ip6_netmask(&bitmask, netmask);
+ }
+#endif
+
+#ifdef IP_SET_HASH_WITH_BITMASK
+ if (tb[IPSET_ATTR_BITMASK]) {
+ /* bitmask and netmask do the same thing, allow only one of these options */
+ if (tb[IPSET_ATTR_NETMASK])
+ return -IPSET_ERR_BITMASK_NETMASK_EXCL;
+
+ if (set->family == NFPROTO_IPV4) {
+ ret = ip_set_get_ipaddr4(tb[IPSET_ATTR_BITMASK], &bitmask.ip);
+ if (ret || !bitmask.ip)
+ return -IPSET_ERR_INVALID_NETMASK;
+ } else if (set->family == NFPROTO_IPV6) {
+ ret = ip_set_get_ipaddr6(tb[IPSET_ATTR_BITMASK], &bitmask);
+ if (ret || ipv6_addr_any(&bitmask.in6))
+ return -IPSET_ERR_INVALID_NETMASK;
+ }
+
+ if (nf_inet_addr_cmp(&bitmask, &zeromask))
return -IPSET_ERR_INVALID_NETMASK;
}
#endif
@@ -1536,7 +1586,8 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
for (i = 0; i < ahash_numof_locks(hbits); i++)
spin_lock_init(&t->hregion[i].lock);
h->maxelem = maxelem;
-#ifdef IP_SET_HASH_WITH_NETMASK
+#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
+ h->bitmask = bitmask;
h->netmask = netmask;
#endif
#ifdef IP_SET_HASH_WITH_MARKMASK
diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
index 75d556d71652..e30513cefd90 100644
--- a/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/net/netfilter/ipset/ip_set_hash_ip.c
@@ -24,7 +24,8 @@
/* 2 Comments support */
/* 3 Forceadd support */
/* 4 skbinfo support */
-#define IPSET_TYPE_REV_MAX 5 /* bucketsize, initval support */
+/* 5 bucketsize, initval support */
+#define IPSET_TYPE_REV_MAX 6 /* bitmask support */
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
@@ -34,6 +35,7 @@ MODULE_ALIAS("ip_set_hash:ip");
/* Type specific function prefix */
#define HTYPE hash_ip
#define IP_SET_HASH_WITH_NETMASK
+#define IP_SET_HASH_WITH_BITMASK
/* IPv4 variant */
@@ -86,7 +88,7 @@ hash_ip4_kadt(struct ip_set *set, const struct sk_buff *skb,
__be32 ip;
ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &ip);
- ip &= ip_set_netmask(h->netmask);
+ ip &= h->bitmask.ip;
if (ip == 0)
return -EINVAL;
@@ -119,7 +121,7 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
- ip &= ip_set_hostmask(h->netmask);
+ ip &= ntohl(h->bitmask.ip);
e.ip = htonl(ip);
if (e.ip == 0)
return -IPSET_ERR_HASH_ELEM;
@@ -185,12 +187,6 @@ hash_ip6_data_equal(const struct hash_ip6_elem *ip1,
return ipv6_addr_equal(&ip1->ip.in6, &ip2->ip.in6);
}
-static void
-hash_ip6_netmask(union nf_inet_addr *ip, u8 prefix)
-{
- ip6_netmask(ip, prefix);
-}
-
static bool
hash_ip6_data_list(struct sk_buff *skb, const struct hash_ip6_elem *e)
{
@@ -227,7 +223,7 @@ hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
- hash_ip6_netmask(&e.ip, h->netmask);
+ nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
if (ipv6_addr_any(&e.ip.in6))
return -EINVAL;
@@ -266,7 +262,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
- hash_ip6_netmask(&e.ip, h->netmask);
+ nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
if (ipv6_addr_any(&e.ip.in6))
return -IPSET_ERR_HASH_ELEM;
@@ -293,6 +289,7 @@ static struct ip_set_type hash_ip_type __read_mostly = {
[IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
[IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
[IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
+ [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
[IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
},
.adt_policy = {
diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
index 7303138e46be..2ffbd0b78a8c 100644
--- a/net/netfilter/ipset/ip_set_hash_ipport.c
+++ b/net/netfilter/ipset/ip_set_hash_ipport.c
@@ -26,7 +26,8 @@
/* 3 Comments support added */
/* 4 Forceadd support added */
/* 5 skbinfo support added */
-#define IPSET_TYPE_REV_MAX 6 /* bucketsize, initval support added */
+/* 6 bucketsize, initval support added */
+#define IPSET_TYPE_REV_MAX 7 /* bitmask support added */
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
@@ -35,6 +36,8 @@ MODULE_ALIAS("ip_set_hash:ip,port");
/* Type specific function prefix */
#define HTYPE hash_ipport
+#define IP_SET_HASH_WITH_NETMASK
+#define IP_SET_HASH_WITH_BITMASK
/* IPv4 variant */
@@ -92,12 +95,16 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
ipset_adtfn adtfn = set->variant->adt[adt];
struct hash_ipport4_elem e = { .ip = 0 };
struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
+ const struct MTYPE *h = set->data;
if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
&e.port, &e.proto))
return -EINVAL;
ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
+ e.ip &= h->bitmask.ip;
+ if (e.ip == 0)
+ return -EINVAL;
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -129,6 +136,10 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
+ e.ip &= h->bitmask.ip;
+ if (e.ip == 0)
+ return -EINVAL;
+
e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
if (tb[IPSET_ATTR_PROTO]) {
@@ -253,12 +264,17 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
ipset_adtfn adtfn = set->variant->adt[adt];
struct hash_ipport6_elem e = { .ip = { .all = { 0 } } };
struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
+ const struct MTYPE *h = set->data;
if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
&e.port, &e.proto))
return -EINVAL;
ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
+ nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
+ if (ipv6_addr_any(&e.ip.in6))
+ return -EINVAL;
+
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -298,6 +314,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
+ nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
+ if (ipv6_addr_any(&e.ip.in6))
+ return -EINVAL;
+
e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
if (tb[IPSET_ATTR_PROTO]) {
@@ -356,6 +376,8 @@ static struct ip_set_type hash_ipport_type __read_mostly = {
[IPSET_ATTR_PROTO] = { .type = NLA_U8 },
[IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
[IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
+ [IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
+ [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
},
.adt_policy = {
[IPSET_ATTR_IP] = { .type = NLA_NESTED },
diff --git a/net/netfilter/ipset/ip_set_hash_netnet.c b/net/netfilter/ipset/ip_set_hash_netnet.c
index 3d09eefe998a..c514ac19486d 100644
--- a/net/netfilter/ipset/ip_set_hash_netnet.c
+++ b/net/netfilter/ipset/ip_set_hash_netnet.c
@@ -23,7 +23,8 @@
#define IPSET_TYPE_REV_MIN 0
/* 1 Forceadd support added */
/* 2 skbinfo support added */
-#define IPSET_TYPE_REV_MAX 3 /* bucketsize, initval support added */
+/* 3 bucketsize, initval support added */
+#define IPSET_TYPE_REV_MAX 4 /* bitmask support added */
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Oliver Smith <oliver@8.c.9.b.0.7.4.0.1.0.0.2.ip6.arpa>");
@@ -33,6 +34,7 @@ MODULE_ALIAS("ip_set_hash:net,net");
/* Type specific function prefix */
#define HTYPE hash_netnet
#define IP_SET_HASH_WITH_NETS
+#define IP_SET_HASH_WITH_BITMASK
#define IPSET_NET_COUNT 2
/* IPv4 variants */
@@ -153,8 +155,8 @@ hash_netnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip[0]);
ip4addrptr(skb, opt->flags & IPSET_DIM_TWO_SRC, &e.ip[1]);
- e.ip[0] &= ip_set_netmask(e.cidr[0]);
- e.ip[1] &= ip_set_netmask(e.cidr[1]);
+ e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
+ e.ip[1] &= (ip_set_netmask(e.cidr[1]) & h->bitmask.ip);
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -213,8 +215,8 @@ hash_netnet4_uadt(struct ip_set *set, struct nlattr *tb[],
if (adt == IPSET_TEST || !(tb[IPSET_ATTR_IP_TO] ||
tb[IPSET_ATTR_IP2_TO])) {
- e.ip[0] = htonl(ip & ip_set_hostmask(e.cidr[0]));
- e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
+ e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[0]));
+ e.ip[1] = htonl(ip2_from & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[1]));
ret = adtfn(set, &e, &ext, &ext, flags);
return ip_set_enomatch(ret, flags, adt, set) ? -ret :
ip_set_eexist(ret, flags) ? 0 : ret;
@@ -404,6 +406,11 @@ hash_netnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
ip6_netmask(&e.ip[0], e.cidr[0]);
ip6_netmask(&e.ip[1], e.cidr[1]);
+ nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
+ nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
+ if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
+ return -EINVAL;
+
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -414,6 +421,7 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
ipset_adtfn adtfn = set->variant->adt[adt];
struct hash_netnet6_elem e = { };
struct ip_set_ext ext = IP_SET_INIT_UEXT(set);
+ const struct hash_netnet6 *h = set->data;
int ret;
if (tb[IPSET_ATTR_LINENO])
@@ -453,6 +461,11 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
ip6_netmask(&e.ip[0], e.cidr[0]);
ip6_netmask(&e.ip[1], e.cidr[1]);
+ nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
+ nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
+ if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
+ return -IPSET_ERR_HASH_ELEM;
+
if (tb[IPSET_ATTR_CADT_FLAGS]) {
u32 cadt_flags = ip_set_get_h32(tb[IPSET_ATTR_CADT_FLAGS]);
@@ -484,6 +497,7 @@ static struct ip_set_type hash_netnet_type __read_mostly = {
[IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
[IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
[IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
+ [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
},
.adt_policy = {
[IPSET_ATTR_IP] = { .type = NLA_NESTED },
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel)
2022-09-28 18:26 ` [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel) Vishwanath Pai
@ 2022-11-07 8:54 ` Jozsef Kadlecsik
2022-11-07 21:48 ` Vishwanath Pai
0 siblings, 1 reply; 5+ messages in thread
From: Jozsef Kadlecsik @ 2022-11-07 8:54 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: pablo, fw, johunt, netfilter-devel
On Wed, 28 Sep 2022, Vishwanath Pai wrote:
> Add a new parameter to complement the existing 'netmask' option. The
> main difference between netmask and bitmask is that bitmask takes any
> arbitrary ip address as input, it does not have to be a valid netmask.
>
> The name of the new parameter is 'bitmask'. This lets us mask out
> arbitrary bits in the ip address, for example:
> ipset create set1 hash:ip bitmask 255.128.255.0
> ipset create set2 hash:ip,port family inet6 bitmask ffff::ff80
>
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> Signed-off-by: Joshua Hunt <johunt@akamai.com>
> ---
>
> Notes:
> Changes in v2 based on code review comments:
> * bitmask and netmask options are mutually exclusive now
> * Added tests for the bitmask feature to all three set types
> * IP_SET_HASH_WITH_BITMASK is a separate macro now
> * netmask is now converted and stored as 'bitmask' and bitmask is
> applied to the entries directly for better efficiency
> * fixed comment for revision 6 in hash:ip
> * removed netmask from hash:net,net, it has a cidr option already
>
> include/linux/netfilter.h | 9 +++
> include/uapi/linux/netfilter/ipset/ip_set.h | 2 +
> net/netfilter/ipset/ip_set_hash_gen.h | 75 +++++++++++++++++----
> net/netfilter/ipset/ip_set_hash_ip.c | 19 +++---
> net/netfilter/ipset/ip_set_hash_ipport.c | 24 ++++++-
> net/netfilter/ipset/ip_set_hash_netnet.c | 24 +++++--
> 6 files changed, 124 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index c2c6f332fb90..c7155eaebc84 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -56,6 +56,15 @@ static inline void nf_inet_addr_mask(const union nf_inet_addr *a1,
> #endif
> }
>
> +static inline void nf_inet_addr_mask_inplace(union nf_inet_addr *a1,
> + const union nf_inet_addr *mask)
> +{
> + a1->all[0] &= mask->all[0];
> + a1->all[1] &= mask->all[1];
> + a1->all[2] &= mask->all[2];
> + a1->all[3] &= mask->all[3];
> +}
> +
I see that the natural place for the new inline function is in
include/linux/netfilter.h but the only user is ipset. So probably it'd be
better to move it into include/linux/netfilter/ipset/ip_set.h. I leave it
up to you.
> int netfilter_init(void);
>
> struct sk_buff;
> diff --git a/include/uapi/linux/netfilter/ipset/ip_set.h b/include/uapi/linux/netfilter/ipset/ip_set.h
> index 6397d75899bc..1bfa765a2191 100644
> --- a/include/uapi/linux/netfilter/ipset/ip_set.h
> +++ b/include/uapi/linux/netfilter/ipset/ip_set.h
> @@ -89,6 +89,7 @@ enum {
> IPSET_ATTR_CADT_LINENO = IPSET_ATTR_LINENO, /* 9 */
> IPSET_ATTR_MARK, /* 10 */
> IPSET_ATTR_MARKMASK, /* 11 */
> + IPSET_ATTR_BITMASK, /* 12 */
> /* Reserve empty slots */
> IPSET_ATTR_CADT_MAX = 16,
> /* Create-only specific attributes */
> @@ -157,6 +158,7 @@ enum ipset_errno {
> IPSET_ERR_COMMENT,
> IPSET_ERR_INVALID_MARKMASK,
> IPSET_ERR_SKBINFO,
> + IPSET_ERR_BITMASK_NETMASK_EXCL,
>
> /* Type specific error codes */
> IPSET_ERR_TYPE_SPECIFIC = 4352,
> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index 6e391308431d..1f1b106a2cd8 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
> @@ -182,6 +182,17 @@ htable_size(u8 hbits)
> (SET_WITH_TIMEOUT(set) && \
> ip_set_timeout_expired(ext_timeout(d, set)))
>
> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
> +static const union nf_inet_addr onesmask = {
> + .all[0] = 0xffffffff,
> + .all[1] = 0xffffffff,
> + .all[2] = 0xffffffff,
> + .all[3] = 0xffffffff
> +};
> +
> +static const union nf_inet_addr zeromask;
I might be paranoid but I'd like to see an explicit setting to zero, i.e.
static const union nf_inet_addr zeromask = {};
> +#endif
> +
> #endif /* _IP_SET_HASH_GEN_H */
>
> #ifndef MTYPE
> @@ -306,8 +317,9 @@ struct htype {
> u32 markmask; /* markmask value for mark mask to store */
> #endif
> u8 bucketsize; /* max elements in an array block */
> -#ifdef IP_SET_HASH_WITH_NETMASK
> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
> u8 netmask; /* netmask value for subnets to store */
> + union nf_inet_addr bitmask; /* stores bitmask */
> #endif
> struct list_head ad; /* Resize add|del backlist */
> struct mtype_elem next; /* temporary storage for uadd */
> @@ -482,8 +494,8 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
> /* Resizing changes htable_bits, so we ignore it */
> return x->maxelem == y->maxelem &&
> a->timeout == b->timeout &&
> -#ifdef IP_SET_HASH_WITH_NETMASK
> - x->netmask == y->netmask &&
> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
> + nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
> #endif
> #ifdef IP_SET_HASH_WITH_MARKMASK
> x->markmask == y->markmask &&
> @@ -1282,9 +1294,21 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
> htonl(jhash_size(htable_bits))) ||
> nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
> goto nla_put_failure;
> +#ifdef IP_SET_HASH_WITH_BITMASK
> + /* if netmask is set to anything other than HOST_MASK we know that the user supplied netmask
> + * and not bitmask. These two are mutually exclusive. */
> + if (h->netmask == HOST_MASK && !nf_inet_addr_cmp(&onesmask, &h->bitmask)) {
> + if (set->family == NFPROTO_IPV4) {
> + if (nla_put_ipaddr4(skb, IPSET_ATTR_BITMASK, h->bitmask.ip))
> + goto nla_put_failure;
> + } else if (set->family == NFPROTO_IPV6) {
> + if (nla_put_ipaddr6(skb, IPSET_ATTR_BITMASK, &h->bitmask.in6))
> + goto nla_put_failure;
> + }
> + }
> +#endif
> #ifdef IP_SET_HASH_WITH_NETMASK
> - if (h->netmask != HOST_MASK &&
> - nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
> + if (h->netmask != HOST_MASK && nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
> goto nla_put_failure;
> #endif
> #ifdef IP_SET_HASH_WITH_MARKMASK
> @@ -1447,8 +1471,10 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> u32 markmask;
> #endif
> u8 hbits;
> -#ifdef IP_SET_HASH_WITH_NETMASK
> - u8 netmask;
> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
> + int ret __attribute__((unused)) = 0;
> + u8 netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
> + union nf_inet_addr bitmask = onesmask;
> #endif
> size_t hsize;
> struct htype *h;
> @@ -1486,13 +1512,37 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> #endif
>
> #ifdef IP_SET_HASH_WITH_NETMASK
> - netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
> if (tb[IPSET_ATTR_NETMASK]) {
> netmask = nla_get_u8(tb[IPSET_ATTR_NETMASK]);
> -
> if ((set->family == NFPROTO_IPV4 && netmask > 32) ||
> - (set->family == NFPROTO_IPV6 && netmask > 128) ||
> - netmask == 0)
Why is the checking against zero value removed?
Best regards,
Jozsef
> + (set->family == NFPROTO_IPV6 && netmask > 128))
> + return -IPSET_ERR_INVALID_NETMASK;
> +
> + /* we convert netmask to bitmask and store it */
> + if (set->family == NFPROTO_IPV4)
> + bitmask.ip = ip_set_netmask(netmask);
> + else
> + ip6_netmask(&bitmask, netmask);
> + }
> +#endif
> +
> +#ifdef IP_SET_HASH_WITH_BITMASK
> + if (tb[IPSET_ATTR_BITMASK]) {
> + /* bitmask and netmask do the same thing, allow only one of these options */
> + if (tb[IPSET_ATTR_NETMASK])
> + return -IPSET_ERR_BITMASK_NETMASK_EXCL;
> +
> + if (set->family == NFPROTO_IPV4) {
> + ret = ip_set_get_ipaddr4(tb[IPSET_ATTR_BITMASK], &bitmask.ip);
> + if (ret || !bitmask.ip)
> + return -IPSET_ERR_INVALID_NETMASK;
> + } else if (set->family == NFPROTO_IPV6) {
> + ret = ip_set_get_ipaddr6(tb[IPSET_ATTR_BITMASK], &bitmask);
> + if (ret || ipv6_addr_any(&bitmask.in6))
> + return -IPSET_ERR_INVALID_NETMASK;
> + }
> +
> + if (nf_inet_addr_cmp(&bitmask, &zeromask))
> return -IPSET_ERR_INVALID_NETMASK;
> }
> #endif
> @@ -1536,7 +1586,8 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> for (i = 0; i < ahash_numof_locks(hbits); i++)
> spin_lock_init(&t->hregion[i].lock);
> h->maxelem = maxelem;
> -#ifdef IP_SET_HASH_WITH_NETMASK
> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
> + h->bitmask = bitmask;
> h->netmask = netmask;
> #endif
> #ifdef IP_SET_HASH_WITH_MARKMASK
> diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
> index 75d556d71652..e30513cefd90 100644
> --- a/net/netfilter/ipset/ip_set_hash_ip.c
> +++ b/net/netfilter/ipset/ip_set_hash_ip.c
> @@ -24,7 +24,8 @@
> /* 2 Comments support */
> /* 3 Forceadd support */
> /* 4 skbinfo support */
> -#define IPSET_TYPE_REV_MAX 5 /* bucketsize, initval support */
> +/* 5 bucketsize, initval support */
> +#define IPSET_TYPE_REV_MAX 6 /* bitmask support */
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
> @@ -34,6 +35,7 @@ MODULE_ALIAS("ip_set_hash:ip");
> /* Type specific function prefix */
> #define HTYPE hash_ip
> #define IP_SET_HASH_WITH_NETMASK
> +#define IP_SET_HASH_WITH_BITMASK
>
> /* IPv4 variant */
>
> @@ -86,7 +88,7 @@ hash_ip4_kadt(struct ip_set *set, const struct sk_buff *skb,
> __be32 ip;
>
> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &ip);
> - ip &= ip_set_netmask(h->netmask);
> + ip &= h->bitmask.ip;
> if (ip == 0)
> return -EINVAL;
>
> @@ -119,7 +121,7 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> - ip &= ip_set_hostmask(h->netmask);
> + ip &= ntohl(h->bitmask.ip);
> e.ip = htonl(ip);
> if (e.ip == 0)
> return -IPSET_ERR_HASH_ELEM;
> @@ -185,12 +187,6 @@ hash_ip6_data_equal(const struct hash_ip6_elem *ip1,
> return ipv6_addr_equal(&ip1->ip.in6, &ip2->ip.in6);
> }
>
> -static void
> -hash_ip6_netmask(union nf_inet_addr *ip, u8 prefix)
> -{
> - ip6_netmask(ip, prefix);
> -}
> -
> static bool
> hash_ip6_data_list(struct sk_buff *skb, const struct hash_ip6_elem *e)
> {
> @@ -227,7 +223,7 @@ hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
>
> ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
> - hash_ip6_netmask(&e.ip, h->netmask);
> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
> if (ipv6_addr_any(&e.ip.in6))
> return -EINVAL;
>
> @@ -266,7 +262,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> - hash_ip6_netmask(&e.ip, h->netmask);
> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
> if (ipv6_addr_any(&e.ip.in6))
> return -IPSET_ERR_HASH_ELEM;
>
> @@ -293,6 +289,7 @@ static struct ip_set_type hash_ip_type __read_mostly = {
> [IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
> [IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
> },
> .adt_policy = {
> diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
> index 7303138e46be..2ffbd0b78a8c 100644
> --- a/net/netfilter/ipset/ip_set_hash_ipport.c
> +++ b/net/netfilter/ipset/ip_set_hash_ipport.c
> @@ -26,7 +26,8 @@
> /* 3 Comments support added */
> /* 4 Forceadd support added */
> /* 5 skbinfo support added */
> -#define IPSET_TYPE_REV_MAX 6 /* bucketsize, initval support added */
> +/* 6 bucketsize, initval support added */
> +#define IPSET_TYPE_REV_MAX 7 /* bitmask support added */
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
> @@ -35,6 +36,8 @@ MODULE_ALIAS("ip_set_hash:ip,port");
>
> /* Type specific function prefix */
> #define HTYPE hash_ipport
> +#define IP_SET_HASH_WITH_NETMASK
> +#define IP_SET_HASH_WITH_BITMASK
>
> /* IPv4 variant */
>
> @@ -92,12 +95,16 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
> ipset_adtfn adtfn = set->variant->adt[adt];
> struct hash_ipport4_elem e = { .ip = 0 };
> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
> + const struct MTYPE *h = set->data;
>
> if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> &e.port, &e.proto))
> return -EINVAL;
>
> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
> + e.ip &= h->bitmask.ip;
> + if (e.ip == 0)
> + return -EINVAL;
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
>
> @@ -129,6 +136,10 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> + e.ip &= h->bitmask.ip;
> + if (e.ip == 0)
> + return -EINVAL;
> +
> e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
>
> if (tb[IPSET_ATTR_PROTO]) {
> @@ -253,12 +264,17 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
> ipset_adtfn adtfn = set->variant->adt[adt];
> struct hash_ipport6_elem e = { .ip = { .all = { 0 } } };
> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
> + const struct MTYPE *h = set->data;
>
> if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> &e.port, &e.proto))
> return -EINVAL;
>
> ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
> + if (ipv6_addr_any(&e.ip.in6))
> + return -EINVAL;
> +
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
>
> @@ -298,6 +314,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
> + if (ipv6_addr_any(&e.ip.in6))
> + return -EINVAL;
> +
> e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
>
> if (tb[IPSET_ATTR_PROTO]) {
> @@ -356,6 +376,8 @@ static struct ip_set_type hash_ipport_type __read_mostly = {
> [IPSET_ATTR_PROTO] = { .type = NLA_U8 },
> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
> + [IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
> },
> .adt_policy = {
> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
> diff --git a/net/netfilter/ipset/ip_set_hash_netnet.c b/net/netfilter/ipset/ip_set_hash_netnet.c
> index 3d09eefe998a..c514ac19486d 100644
> --- a/net/netfilter/ipset/ip_set_hash_netnet.c
> +++ b/net/netfilter/ipset/ip_set_hash_netnet.c
> @@ -23,7 +23,8 @@
> #define IPSET_TYPE_REV_MIN 0
> /* 1 Forceadd support added */
> /* 2 skbinfo support added */
> -#define IPSET_TYPE_REV_MAX 3 /* bucketsize, initval support added */
> +/* 3 bucketsize, initval support added */
> +#define IPSET_TYPE_REV_MAX 4 /* bitmask support added */
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Oliver Smith <oliver@8.c.9.b.0.7.4.0.1.0.0.2.ip6.arpa>");
> @@ -33,6 +34,7 @@ MODULE_ALIAS("ip_set_hash:net,net");
> /* Type specific function prefix */
> #define HTYPE hash_netnet
> #define IP_SET_HASH_WITH_NETS
> +#define IP_SET_HASH_WITH_BITMASK
> #define IPSET_NET_COUNT 2
>
> /* IPv4 variants */
> @@ -153,8 +155,8 @@ hash_netnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
>
> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip[0]);
> ip4addrptr(skb, opt->flags & IPSET_DIM_TWO_SRC, &e.ip[1]);
> - e.ip[0] &= ip_set_netmask(e.cidr[0]);
> - e.ip[1] &= ip_set_netmask(e.cidr[1]);
> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
> + e.ip[1] &= (ip_set_netmask(e.cidr[1]) & h->bitmask.ip);
>
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
> @@ -213,8 +215,8 @@ hash_netnet4_uadt(struct ip_set *set, struct nlattr *tb[],
>
> if (adt == IPSET_TEST || !(tb[IPSET_ATTR_IP_TO] ||
> tb[IPSET_ATTR_IP2_TO])) {
> - e.ip[0] = htonl(ip & ip_set_hostmask(e.cidr[0]));
> - e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
> + e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[0]));
> + e.ip[1] = htonl(ip2_from & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[1]));
> ret = adtfn(set, &e, &ext, &ext, flags);
> return ip_set_enomatch(ret, flags, adt, set) ? -ret :
> ip_set_eexist(ret, flags) ? 0 : ret;
> @@ -404,6 +406,11 @@ hash_netnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
> ip6_netmask(&e.ip[0], e.cidr[0]);
> ip6_netmask(&e.ip[1], e.cidr[1]);
>
> + nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
> + nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
> + if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
> + return -EINVAL;
> +
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
>
> @@ -414,6 +421,7 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
> ipset_adtfn adtfn = set->variant->adt[adt];
> struct hash_netnet6_elem e = { };
> struct ip_set_ext ext = IP_SET_INIT_UEXT(set);
> + const struct hash_netnet6 *h = set->data;
> int ret;
>
> if (tb[IPSET_ATTR_LINENO])
> @@ -453,6 +461,11 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
> ip6_netmask(&e.ip[0], e.cidr[0]);
> ip6_netmask(&e.ip[1], e.cidr[1]);
>
> + nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
> + nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
> + if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
> + return -IPSET_ERR_HASH_ELEM;
> +
> if (tb[IPSET_ATTR_CADT_FLAGS]) {
> u32 cadt_flags = ip_set_get_h32(tb[IPSET_ATTR_CADT_FLAGS]);
>
> @@ -484,6 +497,7 @@ static struct ip_set_type hash_netnet_type __read_mostly = {
> [IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
> },
> .adt_policy = {
> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
> --
> 2.25.1
>
>
-
E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel)
2022-11-07 8:54 ` Jozsef Kadlecsik
@ 2022-11-07 21:48 ` Vishwanath Pai
0 siblings, 0 replies; 5+ messages in thread
From: Vishwanath Pai @ 2022-11-07 21:48 UTC (permalink / raw)
To: Jozsef Kadlecsik; +Cc: pablo, fw, johunt, netfilter-devel
On 11/7/22 03:54, Jozsef Kadlecsik wrote:
> On Wed, 28 Sep 2022, Vishwanath Pai wrote:
>
>> Add a new parameter to complement the existing 'netmask' option. The
>> main difference between netmask and bitmask is that bitmask takes any
>> arbitrary ip address as input, it does not have to be a valid netmask.
>>
>> The name of the new parameter is 'bitmask'. This lets us mask out
>> arbitrary bits in the ip address, for example:
>> ipset create set1 hash:ip bitmask 255.128.255.0
>> ipset create set2 hash:ip,port family inet6 bitmask ffff::ff80
>>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> Signed-off-by: Joshua Hunt <johunt@akamai.com>
>> ---
>>
>> Notes:
>> Changes in v2 based on code review comments:
>> * bitmask and netmask options are mutually exclusive now
>> * Added tests for the bitmask feature to all three set types
>> * IP_SET_HASH_WITH_BITMASK is a separate macro now
>> * netmask is now converted and stored as 'bitmask' and bitmask is
>> applied to the entries directly for better efficiency
>> * fixed comment for revision 6 in hash:ip
>> * removed netmask from hash:net,net, it has a cidr option already
>>
>> include/linux/netfilter.h | 9 +++
>> include/uapi/linux/netfilter/ipset/ip_set.h | 2 +
>> net/netfilter/ipset/ip_set_hash_gen.h | 75 +++++++++++++++++----
>> net/netfilter/ipset/ip_set_hash_ip.c | 19 +++---
>> net/netfilter/ipset/ip_set_hash_ipport.c | 24 ++++++-
>> net/netfilter/ipset/ip_set_hash_netnet.c | 24 +++++--
>> 6 files changed, 124 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
>> index c2c6f332fb90..c7155eaebc84 100644
>> --- a/include/linux/netfilter.h
>> +++ b/include/linux/netfilter.h
>> @@ -56,6 +56,15 @@ static inline void nf_inet_addr_mask(const union nf_inet_addr *a1,
>> #endif
>> }
>>
>> +static inline void nf_inet_addr_mask_inplace(union nf_inet_addr *a1,
>> + const union nf_inet_addr *mask)
>> +{
>> + a1->all[0] &= mask->all[0];
>> + a1->all[1] &= mask->all[1];
>> + a1->all[2] &= mask->all[2];
>> + a1->all[3] &= mask->all[3];
>> +}
>> +
>
> I see that the natural place for the new inline function is in
> include/linux/netfilter.h but the only user is ipset. So probably it'd be
> better to move it into include/linux/netfilter/ipset/ip_set.h. I leave it
> up to you.
>
>> int netfilter_init(void);
>>
>> struct sk_buff;
>> diff --git a/include/uapi/linux/netfilter/ipset/ip_set.h b/include/uapi/linux/netfilter/ipset/ip_set.h
>> index 6397d75899bc..1bfa765a2191 100644
>> --- a/include/uapi/linux/netfilter/ipset/ip_set.h
>> +++ b/include/uapi/linux/netfilter/ipset/ip_set.h
>> @@ -89,6 +89,7 @@ enum {
>> IPSET_ATTR_CADT_LINENO = IPSET_ATTR_LINENO, /* 9 */
>> IPSET_ATTR_MARK, /* 10 */
>> IPSET_ATTR_MARKMASK, /* 11 */
>> + IPSET_ATTR_BITMASK, /* 12 */
>> /* Reserve empty slots */
>> IPSET_ATTR_CADT_MAX = 16,
>> /* Create-only specific attributes */
>> @@ -157,6 +158,7 @@ enum ipset_errno {
>> IPSET_ERR_COMMENT,
>> IPSET_ERR_INVALID_MARKMASK,
>> IPSET_ERR_SKBINFO,
>> + IPSET_ERR_BITMASK_NETMASK_EXCL,
>>
>> /* Type specific error codes */
>> IPSET_ERR_TYPE_SPECIFIC = 4352,
>> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
>> index 6e391308431d..1f1b106a2cd8 100644
>> --- a/net/netfilter/ipset/ip_set_hash_gen.h
>> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
>> @@ -182,6 +182,17 @@ htable_size(u8 hbits)
>> (SET_WITH_TIMEOUT(set) && \
>> ip_set_timeout_expired(ext_timeout(d, set)))
>>
>> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>> +static const union nf_inet_addr onesmask = {
>> + .all[0] = 0xffffffff,
>> + .all[1] = 0xffffffff,
>> + .all[2] = 0xffffffff,
>> + .all[3] = 0xffffffff
>> +};
>> +
>> +static const union nf_inet_addr zeromask;
>
> I might be paranoid but I'd like to see an explicit setting to zero, i.e.
>
> static const union nf_inet_addr zeromask = {};
>
>> +#endif
>> +
>> #endif /* _IP_SET_HASH_GEN_H */
>>
>> #ifndef MTYPE
>> @@ -306,8 +317,9 @@ struct htype {
>> u32 markmask; /* markmask value for mark mask to store */
>> #endif
>> u8 bucketsize; /* max elements in an array block */
>> -#ifdef IP_SET_HASH_WITH_NETMASK
>> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>> u8 netmask; /* netmask value for subnets to store */
>> + union nf_inet_addr bitmask; /* stores bitmask */
>> #endif
>> struct list_head ad; /* Resize add|del backlist */
>> struct mtype_elem next; /* temporary storage for uadd */
>> @@ -482,8 +494,8 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
>> /* Resizing changes htable_bits, so we ignore it */
>> return x->maxelem == y->maxelem &&
>> a->timeout == b->timeout &&
>> -#ifdef IP_SET_HASH_WITH_NETMASK
>> - x->netmask == y->netmask &&
>> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>> + nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
>> #endif
>> #ifdef IP_SET_HASH_WITH_MARKMASK
>> x->markmask == y->markmask &&
>> @@ -1282,9 +1294,21 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
>> htonl(jhash_size(htable_bits))) ||
>> nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
>> goto nla_put_failure;
>> +#ifdef IP_SET_HASH_WITH_BITMASK
>> + /* if netmask is set to anything other than HOST_MASK we know that the user supplied netmask
>> + * and not bitmask. These two are mutually exclusive. */
>> + if (h->netmask == HOST_MASK && !nf_inet_addr_cmp(&onesmask, &h->bitmask)) {
>> + if (set->family == NFPROTO_IPV4) {
>> + if (nla_put_ipaddr4(skb, IPSET_ATTR_BITMASK, h->bitmask.ip))
>> + goto nla_put_failure;
>> + } else if (set->family == NFPROTO_IPV6) {
>> + if (nla_put_ipaddr6(skb, IPSET_ATTR_BITMASK, &h->bitmask.in6))
>> + goto nla_put_failure;
>> + }
>> + }
>> +#endif
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> - if (h->netmask != HOST_MASK &&
>> - nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
>> + if (h->netmask != HOST_MASK && nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
>> goto nla_put_failure;
>> #endif
>> #ifdef IP_SET_HASH_WITH_MARKMASK
>> @@ -1447,8 +1471,10 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> u32 markmask;
>> #endif
>> u8 hbits;
>> -#ifdef IP_SET_HASH_WITH_NETMASK
>> - u8 netmask;
>> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>> + int ret __attribute__((unused)) = 0;
>> + u8 netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
>> + union nf_inet_addr bitmask = onesmask;
>> #endif
>> size_t hsize;
>> struct htype *h;
>> @@ -1486,13 +1512,37 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> #endif
>>
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> - netmask = set->family == NFPROTO_IPV4 ? 32 : 128;
>> if (tb[IPSET_ATTR_NETMASK]) {
>> netmask = nla_get_u8(tb[IPSET_ATTR_NETMASK]);
>> -
>> if ((set->family == NFPROTO_IPV4 && netmask > 32) ||
>> - (set->family == NFPROTO_IPV6 && netmask > 128) ||
>> - netmask == 0)
>
> Why is the checking against zero value removed?
>
> Best regards,
> Jozsef
>
>> + (set->family == NFPROTO_IPV6 && netmask > 128))
>> + return -IPSET_ERR_INVALID_NETMASK;
>> +
>> + /* we convert netmask to bitmask and store it */
>> + if (set->family == NFPROTO_IPV4)
>> + bitmask.ip = ip_set_netmask(netmask);
>> + else
>> + ip6_netmask(&bitmask, netmask);
>> + }
>> +#endif
>> +
>> +#ifdef IP_SET_HASH_WITH_BITMASK
>> + if (tb[IPSET_ATTR_BITMASK]) {
>> + /* bitmask and netmask do the same thing, allow only one of these options */
>> + if (tb[IPSET_ATTR_NETMASK])
>> + return -IPSET_ERR_BITMASK_NETMASK_EXCL;
>> +
>> + if (set->family == NFPROTO_IPV4) {
>> + ret = ip_set_get_ipaddr4(tb[IPSET_ATTR_BITMASK], &bitmask.ip);
>> + if (ret || !bitmask.ip)
>> + return -IPSET_ERR_INVALID_NETMASK;
>> + } else if (set->family == NFPROTO_IPV6) {
>> + ret = ip_set_get_ipaddr6(tb[IPSET_ATTR_BITMASK], &bitmask);
>> + if (ret || ipv6_addr_any(&bitmask.in6))
>> + return -IPSET_ERR_INVALID_NETMASK;
>> + }
>> +
>> + if (nf_inet_addr_cmp(&bitmask, &zeromask))
>> return -IPSET_ERR_INVALID_NETMASK;
>> }
>> #endif
>> @@ -1536,7 +1586,8 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> for (i = 0; i < ahash_numof_locks(hbits); i++)
>> spin_lock_init(&t->hregion[i].lock);
>> h->maxelem = maxelem;
>> -#ifdef IP_SET_HASH_WITH_NETMASK
>> +#if defined(IP_SET_HASH_WITH_NETMASK) || defined(IP_SET_HASH_WITH_BITMASK)
>> + h->bitmask = bitmask;
>> h->netmask = netmask;
>> #endif
>> #ifdef IP_SET_HASH_WITH_MARKMASK
>> diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
>> index 75d556d71652..e30513cefd90 100644
>> --- a/net/netfilter/ipset/ip_set_hash_ip.c
>> +++ b/net/netfilter/ipset/ip_set_hash_ip.c
>> @@ -24,7 +24,8 @@
>> /* 2 Comments support */
>> /* 3 Forceadd support */
>> /* 4 skbinfo support */
>> -#define IPSET_TYPE_REV_MAX 5 /* bucketsize, initval support */
>> +/* 5 bucketsize, initval support */
>> +#define IPSET_TYPE_REV_MAX 6 /* bitmask support */
>>
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
>> @@ -34,6 +35,7 @@ MODULE_ALIAS("ip_set_hash:ip");
>> /* Type specific function prefix */
>> #define HTYPE hash_ip
>> #define IP_SET_HASH_WITH_NETMASK
>> +#define IP_SET_HASH_WITH_BITMASK
>>
>> /* IPv4 variant */
>>
>> @@ -86,7 +88,7 @@ hash_ip4_kadt(struct ip_set *set, const struct sk_buff *skb,
>> __be32 ip;
>>
>> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &ip);
>> - ip &= ip_set_netmask(h->netmask);
>> + ip &= h->bitmask.ip;
>> if (ip == 0)
>> return -EINVAL;
>>
>> @@ -119,7 +121,7 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> - ip &= ip_set_hostmask(h->netmask);
>> + ip &= ntohl(h->bitmask.ip);
>> e.ip = htonl(ip);
>> if (e.ip == 0)
>> return -IPSET_ERR_HASH_ELEM;
>> @@ -185,12 +187,6 @@ hash_ip6_data_equal(const struct hash_ip6_elem *ip1,
>> return ipv6_addr_equal(&ip1->ip.in6, &ip2->ip.in6);
>> }
>>
>> -static void
>> -hash_ip6_netmask(union nf_inet_addr *ip, u8 prefix)
>> -{
>> - ip6_netmask(ip, prefix);
>> -}
>> -
>> static bool
>> hash_ip6_data_list(struct sk_buff *skb, const struct hash_ip6_elem *e)
>> {
>> @@ -227,7 +223,7 @@ hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
>> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
>>
>> ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
>> - hash_ip6_netmask(&e.ip, h->netmask);
>> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
>> if (ipv6_addr_any(&e.ip.in6))
>> return -EINVAL;
>>
>> @@ -266,7 +262,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> - hash_ip6_netmask(&e.ip, h->netmask);
>> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
>> if (ipv6_addr_any(&e.ip.in6))
>> return -IPSET_ERR_HASH_ELEM;
>>
>> @@ -293,6 +289,7 @@ static struct ip_set_type hash_ip_type __read_mostly = {
>> [IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
>> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
>> [IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
>> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
>> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
>> },
>> .adt_policy = {
>> diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
>> index 7303138e46be..2ffbd0b78a8c 100644
>> --- a/net/netfilter/ipset/ip_set_hash_ipport.c
>> +++ b/net/netfilter/ipset/ip_set_hash_ipport.c
>> @@ -26,7 +26,8 @@
>> /* 3 Comments support added */
>> /* 4 Forceadd support added */
>> /* 5 skbinfo support added */
>> -#define IPSET_TYPE_REV_MAX 6 /* bucketsize, initval support added */
>> +/* 6 bucketsize, initval support added */
>> +#define IPSET_TYPE_REV_MAX 7 /* bitmask support added */
>>
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
>> @@ -35,6 +36,8 @@ MODULE_ALIAS("ip_set_hash:ip,port");
>>
>> /* Type specific function prefix */
>> #define HTYPE hash_ipport
>> +#define IP_SET_HASH_WITH_NETMASK
>> +#define IP_SET_HASH_WITH_BITMASK
>>
>> /* IPv4 variant */
>>
>> @@ -92,12 +95,16 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
>> ipset_adtfn adtfn = set->variant->adt[adt];
>> struct hash_ipport4_elem e = { .ip = 0 };
>> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
>> + const struct MTYPE *h = set->data;
>>
>> if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>> &e.port, &e.proto))
>> return -EINVAL;
>>
>> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip);
>> + e.ip &= h->bitmask.ip;
>> + if (e.ip == 0)
>> + return -EINVAL;
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>>
>> @@ -129,6 +136,10 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> + e.ip &= h->bitmask.ip;
>> + if (e.ip == 0)
>> + return -EINVAL;
>> +
>> e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
>>
>> if (tb[IPSET_ATTR_PROTO]) {
>> @@ -253,12 +264,17 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
>> ipset_adtfn adtfn = set->variant->adt[adt];
>> struct hash_ipport6_elem e = { .ip = { .all = { 0 } } };
>> struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
>> + const struct MTYPE *h = set->data;
>>
>> if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>> &e.port, &e.proto))
>> return -EINVAL;
>>
>> ip6addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip.in6);
>> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
>> + if (ipv6_addr_any(&e.ip.in6))
>> + return -EINVAL;
>> +
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>>
>> @@ -298,6 +314,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> + nf_inet_addr_mask_inplace(&e.ip, &h->bitmask);
>> + if (ipv6_addr_any(&e.ip.in6))
>> + return -EINVAL;
>> +
>> e.port = nla_get_be16(tb[IPSET_ATTR_PORT]);
>>
>> if (tb[IPSET_ATTR_PROTO]) {
>> @@ -356,6 +376,8 @@ static struct ip_set_type hash_ipport_type __read_mostly = {
>> [IPSET_ATTR_PROTO] = { .type = NLA_U8 },
>> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
>> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
>> + [IPSET_ATTR_NETMASK] = { .type = NLA_U8 },
>> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
>> },
>> .adt_policy = {
>> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
>> diff --git a/net/netfilter/ipset/ip_set_hash_netnet.c b/net/netfilter/ipset/ip_set_hash_netnet.c
>> index 3d09eefe998a..c514ac19486d 100644
>> --- a/net/netfilter/ipset/ip_set_hash_netnet.c
>> +++ b/net/netfilter/ipset/ip_set_hash_netnet.c
>> @@ -23,7 +23,8 @@
>> #define IPSET_TYPE_REV_MIN 0
>> /* 1 Forceadd support added */
>> /* 2 skbinfo support added */
>> -#define IPSET_TYPE_REV_MAX 3 /* bucketsize, initval support added */
>> +/* 3 bucketsize, initval support added */
>> +#define IPSET_TYPE_REV_MAX 4 /* bitmask support added */
>>
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Oliver Smith <oliver@8.c.9.b.0.7.4.0.1.0.0.2.ip6.arpa>");
>> @@ -33,6 +34,7 @@ MODULE_ALIAS("ip_set_hash:net,net");
>> /* Type specific function prefix */
>> #define HTYPE hash_netnet
>> #define IP_SET_HASH_WITH_NETS
>> +#define IP_SET_HASH_WITH_BITMASK
>> #define IPSET_NET_COUNT 2
>>
>> /* IPv4 variants */
>> @@ -153,8 +155,8 @@ hash_netnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
>>
>> ip4addrptr(skb, opt->flags & IPSET_DIM_ONE_SRC, &e.ip[0]);
>> ip4addrptr(skb, opt->flags & IPSET_DIM_TWO_SRC, &e.ip[1]);
>> - e.ip[0] &= ip_set_netmask(e.cidr[0]);
>> - e.ip[1] &= ip_set_netmask(e.cidr[1]);
>> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
>> + e.ip[1] &= (ip_set_netmask(e.cidr[1]) & h->bitmask.ip);
>>
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>> @@ -213,8 +215,8 @@ hash_netnet4_uadt(struct ip_set *set, struct nlattr *tb[],
>>
>> if (adt == IPSET_TEST || !(tb[IPSET_ATTR_IP_TO] ||
>> tb[IPSET_ATTR_IP2_TO])) {
>> - e.ip[0] = htonl(ip & ip_set_hostmask(e.cidr[0]));
>> - e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
>> + e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[0]));
>> + e.ip[1] = htonl(ip2_from & ntohl(h->bitmask.ip) & ip_set_hostmask(e.cidr[1]));
>> ret = adtfn(set, &e, &ext, &ext, flags);
>> return ip_set_enomatch(ret, flags, adt, set) ? -ret :
>> ip_set_eexist(ret, flags) ? 0 : ret;
>> @@ -404,6 +406,11 @@ hash_netnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
>> ip6_netmask(&e.ip[0], e.cidr[0]);
>> ip6_netmask(&e.ip[1], e.cidr[1]);
>>
>> + nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
>> + nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
>> + if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
>> + return -EINVAL;
>> +
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>>
>> @@ -414,6 +421,7 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
>> ipset_adtfn adtfn = set->variant->adt[adt];
>> struct hash_netnet6_elem e = { };
>> struct ip_set_ext ext = IP_SET_INIT_UEXT(set);
>> + const struct hash_netnet6 *h = set->data;
>> int ret;
>>
>> if (tb[IPSET_ATTR_LINENO])
>> @@ -453,6 +461,11 @@ hash_netnet6_uadt(struct ip_set *set, struct nlattr *tb[],
>> ip6_netmask(&e.ip[0], e.cidr[0]);
>> ip6_netmask(&e.ip[1], e.cidr[1]);
>>
>> + nf_inet_addr_mask_inplace(&e.ip[0], &h->bitmask);
>> + nf_inet_addr_mask_inplace(&e.ip[1], &h->bitmask);
>> + if (e.cidr[0] == HOST_MASK && ipv6_addr_any(&e.ip[0].in6))
>> + return -IPSET_ERR_HASH_ELEM;
>> +
>> if (tb[IPSET_ATTR_CADT_FLAGS]) {
>> u32 cadt_flags = ip_set_get_h32(tb[IPSET_ATTR_CADT_FLAGS]);
>>
>> @@ -484,6 +497,7 @@ static struct ip_set_type hash_netnet_type __read_mostly = {
>> [IPSET_ATTR_RESIZE] = { .type = NLA_U8 },
>> [IPSET_ATTR_TIMEOUT] = { .type = NLA_U32 },
>> [IPSET_ATTR_CADT_FLAGS] = { .type = NLA_U32 },
>> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
>> },
>> .adt_policy = {
>> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
>> --
>> 2.25.1
>>
>>
>
> -
> E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
> PGP key : https://urldefense.com/v3/__https://wigner.hu/*kadlec/pgp_public_key.txt__;fg!!GjvTz_vk!Xn_l6E1kmVXKtEbuYab4N5VJx_U3sgMqLvuIZRo0CmSCwHfx92iRGEVzSAfVpHZfJCxHOcNwrG9miA$
> Address : Wigner Research Centre for Physics
> H-1525 Budapest 114, POB. 49, Hungary
Hi Jozsef,
Thanks for taking a look at the patches.
> Why is the checking against zero value removed?
Ah, yes this shouldn't have been removed. I removed it because I set netmask to 32/64 above, but
here we are fetching the value from userspace so we should check for zero. I will fix it.
I will address the other comments and send a v3 in a couple of days.
Thanks,
Vishwanath
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c
2022-09-28 18:26 [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
2022-09-28 18:26 ` [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel) Vishwanath Pai
@ 2022-11-21 13:58 ` Pablo Neira Ayuso
1 sibling, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2022-11-21 13:58 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: kadlec, fw, johunt, netfilter-devel
On Wed, Sep 28, 2022 at 02:26:50PM -0400, Vishwanath Pai wrote:
> This patch introduced a regression: commit 48596a8ddc46 ("netfilter:
> ipset: Fix adding an IPv4 range containing more than 2^31 addresses")
>
> The variable e.ip is passed to adtfn() function which finally adds the
> ip address to the set. The patch above refactored the for loop and moved
> e.ip = htonl(ip) to the end of the for loop.
>
> What this means is that if the value of "ip" changes between the first
> assignement of e.ip and the forloop, then e.ip is pointing to a
> different ip address than "ip".
>
> Test case:
> $ ipset create jdtest_tmp hash:ip family inet hashsize 2048 maxelem 100000
> $ ipset add jdtest_tmp 10.0.1.1/31
> ipset v6.21.1: Element cannot be added to the set: it's already added
>
> The value of ip gets updated inside the "else if (tb[IPSET_ATTR_CIDR])"
> block but e.ip is still pointing to the old value.
Applied to nf.git, thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-21 13:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-28 18:26 [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
2022-09-28 18:26 ` [PATCH v2] netfilter: ipset: Add support for new bitmask parameter (kernel) Vishwanath Pai
2022-11-07 8:54 ` Jozsef Kadlecsik
2022-11-07 21:48 ` Vishwanath Pai
2022-11-21 13:58 ` [PATCH v2] netfilter: ipset: regression in ip_set_hash_ip.c Pablo Neira Ayuso
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).