* [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types
@ 2022-06-29 21:21 Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vishwanath Pai @ 2022-06-29 21:21 UTC (permalink / raw)
To: pablo, kadlec, fw; +Cc: Vishwanath Pai, johunt, netfilter-devel
We fixed a similar problem before in commit 7f4f7dd4417d ("netfilter:
ipset: ipset list may return wrong member count for set with timeout").
The same issue exists in ip_set_bitmap_gen.h as well.
Test case:
$ ipset create test bitmap:ip range 10.0.0.0/8 netmask 24 timeout 5
$ ipset add test 10.0.0.0
$ ipset add test 10.255.255.255
$ sleep 5s
$ ipset list test
Name: test
Type: bitmap:ip
Revision: 3
Header: range 10.0.0.0-10.255.255.255 netmask 24 timeout 5
Size in memory: 532568
References: 0
Number of entries: 2
Members:
We return "Number of entries: 2" but no members are listed. That is
because when we run mtype_head the garbage collector hasn't run yet, but
mtype_list checks and cleans up members with expired timeout. To avoid
this we can run mtype_expire before printing the number of elements in
mytype_head().
Reviewed-by: Joshua Hunt <johunt@akamai.com>
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
---
net/netfilter/ipset/ip_set_bitmap_gen.h | 46 ++++++++++++++++++-------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/net/netfilter/ipset/ip_set_bitmap_gen.h b/net/netfilter/ipset/ip_set_bitmap_gen.h
index 26ab0e9612d8..dd871305bd6e 100644
--- a/net/netfilter/ipset/ip_set_bitmap_gen.h
+++ b/net/netfilter/ipset/ip_set_bitmap_gen.h
@@ -28,6 +28,7 @@
#define mtype_del IPSET_TOKEN(MTYPE, _del)
#define mtype_list IPSET_TOKEN(MTYPE, _list)
#define mtype_gc IPSET_TOKEN(MTYPE, _gc)
+#define mtype_expire IPSET_TOKEN(MTYPE, _expire)
#define mtype MTYPE
#define get_ext(set, map, id) ((map)->extensions + ((set)->dsize * (id)))
@@ -88,13 +89,44 @@ mtype_memsize(const struct mtype *map, size_t dsize)
map->elements * dsize;
}
+/* We should grab set->lock before calling this function */
+static void
+mtype_expire(struct ip_set *set)
+{
+ struct mtype *map = set->data;
+ void *x;
+ u32 id;
+
+ for (id = 0; id < map->elements; id++)
+ if (mtype_gc_test(id, map, set->dsize)) {
+ x = get_ext(set, map, id);
+ if (ip_set_timeout_expired(ext_timeout(x, set))) {
+ clear_bit(id, map->members);
+ ip_set_ext_destroy(set, x);
+ set->elements--;
+ }
+ }
+}
+
static int
mtype_head(struct ip_set *set, struct sk_buff *skb)
{
const struct mtype *map = set->data;
struct nlattr *nested;
- size_t memsize = mtype_memsize(map, set->dsize) + set->ext_size;
+ size_t memsize;
+
+ /* If any members have expired, set->elements will be wrong,
+ * mytype_expire function will update it with the right count.
+ * set->elements can still be incorrect in the case of a huge set
+ * because elements can timeout during set->list().
+ */
+ if (SET_WITH_TIMEOUT(set)) {
+ spin_lock_bh(&set->lock);
+ mtype_expire(set);
+ spin_unlock_bh(&set->lock);
+ }
+ memsize = mtype_memsize(map, set->dsize) + set->ext_size;
nested = nla_nest_start(skb, IPSET_ATTR_DATA);
if (!nested)
goto nla_put_failure;
@@ -266,22 +298,12 @@ mtype_gc(struct timer_list *t)
{
struct mtype *map = from_timer(map, t, gc);
struct ip_set *set = map->set;
- void *x;
- u32 id;
/* We run parallel with other readers (test element)
* but adding/deleting new entries is locked out
*/
spin_lock_bh(&set->lock);
- for (id = 0; id < map->elements; id++)
- if (mtype_gc_test(id, map, set->dsize)) {
- x = get_ext(set, map, id);
- if (ip_set_timeout_expired(ext_timeout(x, set))) {
- clear_bit(id, map->members);
- ip_set_ext_destroy(set, x);
- set->elements--;
- }
- }
+ mtype_expire(set);
spin_unlock_bh(&set->lock);
map->gc.expires = jiffies + IPSET_GC_PERIOD(set->timeout) * HZ;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c
2022-06-29 21:21 [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Vishwanath Pai
@ 2022-06-29 21:21 ` Vishwanath Pai
2022-07-12 21:42 ` Jozsef Kadlecsik
2022-06-29 21:21 ` [PATCH] netfilter: ipset: Add support for new bitmask parameter Vishwanath Pai
2022-07-12 21:06 ` [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Jozsef Kadlecsik
2 siblings, 1 reply; 10+ messages in thread
From: Vishwanath Pai @ 2022-06-29 21:21 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>
---
net/netfilter/ipset/ip_set_hash_ip.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
index dd30c03d5a23..258aeb324483 100644
--- a/net/netfilter/ipset/ip_set_hash_ip.c
+++ b/net/netfilter/ipset/ip_set_hash_ip.c
@@ -120,12 +120,14 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
return ret;
ip &= ip_set_hostmask(h->netmask);
- e.ip = htonl(ip);
- if (e.ip == 0)
+
+ if (ip == 0)
return -IPSET_ERR_HASH_ELEM;
- if (adt == IPSET_TEST)
+ if (adt == IPSET_TEST) {
+ e.ip = htonl(ip);
return adtfn(set, &e, &ext, &ext, flags);
+ }
ip_to = ip;
if (tb[IPSET_ATTR_IP_TO]) {
@@ -145,6 +147,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
ip_set_mask_from_to(ip, ip_to, cidr);
}
+ e.ip = htonl(ip);
+ if (e.ip == 0)
+ return -IPSET_ERR_HASH_ELEM;
+
hosts = h->netmask == 32 ? 1 : 2 << (32 - h->netmask - 1);
/* 64bit division is not allowed on 32bit */
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] netfilter: ipset: Add support for new bitmask parameter
2022-06-29 21:21 [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
@ 2022-06-29 21:21 ` Vishwanath Pai
2022-07-12 22:45 ` Jozsef Kadlecsik
2022-07-12 21:06 ` [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Jozsef Kadlecsik
2 siblings, 1 reply; 10+ messages in thread
From: Vishwanath Pai @ 2022-06-29 21:21 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>
---
include/linux/netfilter.h | 18 ++++++++
include/uapi/linux/netfilter/ipset/ip_set.h | 1 +
net/netfilter/ipset/ip_set_hash_gen.h | 48 ++++++++++++++++++---
net/netfilter/ipset/ip_set_hash_ip.c | 36 +++++++++++-----
net/netfilter/ipset/ip_set_hash_ipport.c | 39 ++++++++++++++++-
net/netfilter/ipset/ip_set_hash_netnet.c | 39 +++++++++++++++--
6 files changed, 160 insertions(+), 21 deletions(-)
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index c2c6f332fb90..c184f01920e2 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -56,6 +56,24 @@ 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];
+}
+
+static inline void nf_inet_addr_invert(const union nf_inet_addr *src,
+ union nf_inet_addr *dst)
+{
+ dst->all[0] = ~src->all[0];
+ dst->all[1] = ~src->all[1];
+ dst->all[2] = ~src->all[2];
+ dst->all[3] = ~src->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..8c3046ea2362 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 */
diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 6e391308431d..1f658b4546b6 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)))
+#ifdef IP_SET_HASH_WITH_NETMASK
+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
@@ -308,6 +319,7 @@ struct htype {
u8 bucketsize; /* max elements in an array block */
#ifdef IP_SET_HASH_WITH_NETMASK
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 */
@@ -484,6 +496,7 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
a->timeout == b->timeout &&
#ifdef IP_SET_HASH_WITH_NETMASK
x->netmask == y->netmask &&
+ nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
#endif
#ifdef IP_SET_HASH_WITH_MARKMASK
x->markmask == y->markmask &&
@@ -1283,8 +1296,16 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
goto nla_put_failure;
#ifdef IP_SET_HASH_WITH_NETMASK
- if (h->netmask != HOST_MASK &&
- nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
+ if (!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;
+ }
+ }
+ 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
@@ -1448,7 +1469,9 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
#endif
u8 hbits;
#ifdef IP_SET_HASH_WITH_NETMASK
- u8 netmask;
+ int ret = 0;
+ u8 netmask = 0;
+ union nf_inet_addr bitmask = onesmask;
#endif
size_t hsize;
struct htype *h;
@@ -1489,10 +1512,22 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
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;
+ }
+ if (tb[IPSET_ATTR_BITMASK]) {
+ 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
@@ -1537,6 +1572,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
spin_lock_init(&t->hregion[i].lock);
h->maxelem = maxelem;
#ifdef IP_SET_HASH_WITH_NETMASK
+ 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 258aeb324483..6976b4bcaa96 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 /* bucketsize, initval support */
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
@@ -86,7 +87,12 @@ 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);
+
+ if (h->netmask != HOST_MASK)
+ ip &= ip_set_netmask(h->netmask);
+ else
+ ip &= h->bitmask.ip;
+
if (ip == 0)
return -EINVAL;
@@ -119,7 +125,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
- ip &= ip_set_hostmask(h->netmask);
+ if (h->netmask != HOST_MASK)
+ ip &= ip_set_hostmask(h->netmask);
+ else
+ ip &= ntohl(h->bitmask.ip);
if (ip == 0)
return -IPSET_ERR_HASH_ELEM;
@@ -193,12 +202,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)
{
@@ -224,6 +227,16 @@ hash_ip6_data_next(struct hash_ip6_elem *next, const struct hash_ip6_elem *e)
#define IP_SET_EMIT_CREATE
#include "ip_set_hash_gen.h"
+static void
+hash_ip6_netmask(union nf_inet_addr *ip, u8 netmask,
+ const union nf_inet_addr *bitmask)
+{
+ if (netmask != HOST_MASK)
+ ip6_netmask(ip, netmask);
+ else
+ nf_inet_addr_mask_inplace(ip, bitmask);
+}
+
static int
hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
const struct xt_action_param *par,
@@ -235,7 +248,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);
+ hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
if (ipv6_addr_any(&e.ip.in6))
return -EINVAL;
@@ -274,7 +287,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
- hash_ip6_netmask(&e.ip, h->netmask);
+ hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
if (ipv6_addr_any(&e.ip.in6))
return -IPSET_ERR_HASH_ELEM;
@@ -301,6 +314,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..8f5379738ffa 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,7 @@ MODULE_ALIAS("ip_set_hash:ip,port");
/* Type specific function prefix */
#define HTYPE hash_ipport
+#define IP_SET_HASH_WITH_NETMASK
/* IPv4 variant */
@@ -92,12 +94,19 @@ 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);
+ if (h->netmask != HOST_MASK)
+ e.ip &= ip_set_netmask(h->netmask);
+ else
+ e.ip &= h->bitmask.ip;
+ if (e.ip == 0)
+ return -EINVAL;
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -129,6 +138,13 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
+ if (h->netmask != HOST_MASK)
+ e.ip &= ip_set_netmask(h->netmask);
+ else
+ 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]) {
@@ -245,6 +261,16 @@ hash_ipport6_data_next(struct hash_ipport6_elem *next,
#define IP_SET_EMIT_CREATE
#include "ip_set_hash_gen.h"
+static void
+hash_ipport6_netmask(union nf_inet_addr *ip, u8 netmask,
+ const union nf_inet_addr *bitmask)
+{
+ if (netmask != HOST_MASK)
+ ip6_netmask(ip, netmask);
+ else
+ nf_inet_addr_mask_inplace(ip, bitmask);
+}
+
static int
hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
const struct xt_action_param *par,
@@ -253,12 +279,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);
+ hash_ipport6_netmask(&e.ip, h->netmask, &h->bitmask);
+ if (ipv6_addr_any(&e.ip.in6))
+ return -EINVAL;
+
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
}
@@ -298,6 +329,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
if (ret)
return ret;
+ hash_ipport6_netmask(&e.ip, h->netmask, &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 +391,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..a44d71cbc192 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_NETMASK
#define IPSET_NET_COUNT 2
/* IPv4 variants */
@@ -153,7 +155,10 @@ 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]);
+ if (h->netmask != HOST_MASK)
+ e.ip[0] &= (ip_set_netmask(e.cidr[0]) & ip_set_netmask(h->netmask));
+ else
+ e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
e.ip[1] &= ip_set_netmask(e.cidr[1]);
return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
@@ -213,7 +218,13 @@ 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]));
+ if (h->netmask != HOST_MASK)
+ e.ip[0] = htonl(ip & ip_set_hostmask(h->netmask)) &
+ ip_set_hostmask(e.cidr[0]);
+ else
+ e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) &
+ ip_set_hostmask(e.cidr[0]));
+
e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
ret = adtfn(set, &e, &ext, &ext, flags);
return ip_set_enomatch(ret, flags, adt, set) ? -ret :
@@ -377,6 +388,16 @@ hash_netnet6_data_next(struct hash_netnet6_elem *next,
#define IP_SET_EMIT_CREATE
#include "ip_set_hash_gen.h"
+static inline void
+hash_netnet6_netmask(union nf_inet_addr *ip, u8 netmask,
+ const union nf_inet_addr *bitmask)
+{
+ if (netmask != HOST_MASK)
+ ip6_netmask(ip, netmask);
+ else
+ nf_inet_addr_mask_inplace(ip, bitmask);
+}
+
static void
hash_netnet6_init(struct hash_netnet6_elem *e)
{
@@ -404,6 +425,10 @@ 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]);
+ hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +439,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 +479,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]);
+ hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +515,8 @@ 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_NETMASK] = { .type = NLA_U8 },
+ [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
},
.adt_policy = {
[IPSET_ATTR_IP] = { .type = NLA_NESTED },
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types
2022-06-29 21:21 [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: Add support for new bitmask parameter Vishwanath Pai
@ 2022-07-12 21:06 ` Jozsef Kadlecsik
2022-07-14 0:08 ` Vishwanath Pai
2 siblings, 1 reply; 10+ messages in thread
From: Jozsef Kadlecsik @ 2022-07-12 21:06 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: pablo, fw, johunt, netfilter-devel
Hi Vishwanath,
On Wed, 29 Jun 2022, Vishwanath Pai wrote:
> We fixed a similar problem before in commit 7f4f7dd4417d ("netfilter:
> ipset: ipset list may return wrong member count for set with timeout").
> The same issue exists in ip_set_bitmap_gen.h as well.
Could you rework the patch to solve the issue the same way as for the hash
types (i.e. scanning the set without locking) like in the commit
33f08da28324 (netfilter: ipset: Fix "INFO: rcu detected stall in hash_xxx"
reports)? I know the bitmap types have got a limited size but it'd be
great if the general method would be the same across the different types.
Best regards,
Jozsef
> Test case:
>
> $ ipset create test bitmap:ip range 10.0.0.0/8 netmask 24 timeout 5
> $ ipset add test 10.0.0.0
> $ ipset add test 10.255.255.255
> $ sleep 5s
>
> $ ipset list test
> Name: test
> Type: bitmap:ip
> Revision: 3
> Header: range 10.0.0.0-10.255.255.255 netmask 24 timeout 5
> Size in memory: 532568
> References: 0
> Number of entries: 2
> Members:
>
> We return "Number of entries: 2" but no members are listed. That is
> because when we run mtype_head the garbage collector hasn't run yet, but
> mtype_list checks and cleans up members with expired timeout. To avoid
> this we can run mtype_expire before printing the number of elements in
> mytype_head().
>
> Reviewed-by: Joshua Hunt <johunt@akamai.com>
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> ---
> net/netfilter/ipset/ip_set_bitmap_gen.h | 46 ++++++++++++++++++-------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_bitmap_gen.h b/net/netfilter/ipset/ip_set_bitmap_gen.h
> index 26ab0e9612d8..dd871305bd6e 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_gen.h
> +++ b/net/netfilter/ipset/ip_set_bitmap_gen.h
> @@ -28,6 +28,7 @@
> #define mtype_del IPSET_TOKEN(MTYPE, _del)
> #define mtype_list IPSET_TOKEN(MTYPE, _list)
> #define mtype_gc IPSET_TOKEN(MTYPE, _gc)
> +#define mtype_expire IPSET_TOKEN(MTYPE, _expire)
> #define mtype MTYPE
>
> #define get_ext(set, map, id) ((map)->extensions + ((set)->dsize * (id)))
> @@ -88,13 +89,44 @@ mtype_memsize(const struct mtype *map, size_t dsize)
> map->elements * dsize;
> }
>
> +/* We should grab set->lock before calling this function */
> +static void
> +mtype_expire(struct ip_set *set)
> +{
> + struct mtype *map = set->data;
> + void *x;
> + u32 id;
> +
> + for (id = 0; id < map->elements; id++)
> + if (mtype_gc_test(id, map, set->dsize)) {
> + x = get_ext(set, map, id);
> + if (ip_set_timeout_expired(ext_timeout(x, set))) {
> + clear_bit(id, map->members);
> + ip_set_ext_destroy(set, x);
> + set->elements--;
> + }
> + }
> +}
> +
> static int
> mtype_head(struct ip_set *set, struct sk_buff *skb)
> {
> const struct mtype *map = set->data;
> struct nlattr *nested;
> - size_t memsize = mtype_memsize(map, set->dsize) + set->ext_size;
> + size_t memsize;
> +
> + /* If any members have expired, set->elements will be wrong,
> + * mytype_expire function will update it with the right count.
> + * set->elements can still be incorrect in the case of a huge set
> + * because elements can timeout during set->list().
> + */
> + if (SET_WITH_TIMEOUT(set)) {
> + spin_lock_bh(&set->lock);
> + mtype_expire(set);
> + spin_unlock_bh(&set->lock);
> + }
>
> + memsize = mtype_memsize(map, set->dsize) + set->ext_size;
> nested = nla_nest_start(skb, IPSET_ATTR_DATA);
> if (!nested)
> goto nla_put_failure;
> @@ -266,22 +298,12 @@ mtype_gc(struct timer_list *t)
> {
> struct mtype *map = from_timer(map, t, gc);
> struct ip_set *set = map->set;
> - void *x;
> - u32 id;
>
> /* We run parallel with other readers (test element)
> * but adding/deleting new entries is locked out
> */
> spin_lock_bh(&set->lock);
> - for (id = 0; id < map->elements; id++)
> - if (mtype_gc_test(id, map, set->dsize)) {
> - x = get_ext(set, map, id);
> - if (ip_set_timeout_expired(ext_timeout(x, set))) {
> - clear_bit(id, map->members);
> - ip_set_ext_destroy(set, x);
> - set->elements--;
> - }
> - }
> + mtype_expire(set);
> spin_unlock_bh(&set->lock);
>
> map->gc.expires = jiffies + IPSET_GC_PERIOD(set->timeout) * HZ;
> --
> 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] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c
2022-06-29 21:21 ` [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
@ 2022-07-12 21:42 ` Jozsef Kadlecsik
2022-07-13 20:07 ` Vishwanath Pai
0 siblings, 1 reply; 10+ messages in thread
From: Jozsef Kadlecsik @ 2022-07-12 21:42 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: pablo, fw, johunt, netfilter-devel
Hi Vishwanath,
On Wed, 29 Jun 2022, 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.
>
> Reviewed-by: Joshua Hunt <johunt@akamai.com>
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> ---
> net/netfilter/ipset/ip_set_hash_ip.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
> index dd30c03d5a23..258aeb324483 100644
> --- a/net/netfilter/ipset/ip_set_hash_ip.c
> +++ b/net/netfilter/ipset/ip_set_hash_ip.c
> @@ -120,12 +120,14 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
> return ret;
>
> ip &= ip_set_hostmask(h->netmask);
> - e.ip = htonl(ip);
> - if (e.ip == 0)
> +
> + if (ip == 0)
> return -IPSET_ERR_HASH_ELEM;
>
> - if (adt == IPSET_TEST)
> + if (adt == IPSET_TEST) {
> + e.ip = htonl(ip);
> return adtfn(set, &e, &ext, &ext, flags);
> + }
>
> ip_to = ip;
> if (tb[IPSET_ATTR_IP_TO]) {
> @@ -145,6 +147,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
> ip_set_mask_from_to(ip, ip_to, cidr);
> }
>
> + e.ip = htonl(ip);
> + if (e.ip == 0)
> + return -IPSET_ERR_HASH_ELEM;
> +
> hosts = h->netmask == 32 ? 1 : 2 << (32 - h->netmask - 1);
>
> /* 64bit division is not allowed on 32bit */
You are right, however the patch can be made much smaller if the e.ip
conversion is simply moved from
if (retried) {
ip = ntohl(h->next.ip);
e.ip = htonl(ip);
}
into the next for loop as the first instruction. Could you resend
your patch that way?
Best regards,
Jozsef
-
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] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: Add support for new bitmask parameter
2022-06-29 21:21 ` [PATCH] netfilter: ipset: Add support for new bitmask parameter Vishwanath Pai
@ 2022-07-12 22:45 ` Jozsef Kadlecsik
2022-07-18 15:18 ` Vishwanath Pai
0 siblings, 1 reply; 10+ messages in thread
From: Jozsef Kadlecsik @ 2022-07-12 22:45 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Pablo Neira Ayuso, Florian Westphal, johunt, netfilter-devel
Hi,
On Wed, 29 Jun 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
In my opinion new stuff should be added to nftables and not to ipset,
but see my comments below
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> Signed-off-by: Joshua Hunt <johunt@akamai.com>
> ---
> include/linux/netfilter.h | 18 ++++++++
> include/uapi/linux/netfilter/ipset/ip_set.h | 1 +
> net/netfilter/ipset/ip_set_hash_gen.h | 48 ++++++++++++++++++---
> net/netfilter/ipset/ip_set_hash_ip.c | 36 +++++++++++-----
> net/netfilter/ipset/ip_set_hash_ipport.c | 39 ++++++++++++++++-
> net/netfilter/ipset/ip_set_hash_netnet.c | 39 +++++++++++++++--
Why these three set types are picked? If you introduce a new feature, add
it to all possible set types.
What does the bitmask option exactly means for the hash:net* types? It is
a set level option and not a per entry level one, so the functionality is
limited.
The netmask and bitmap options are excluding ones, so those should be
handled that way.
Please add tests to the testsuite to verify the functionality.
> 6 files changed, 160 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index c2c6f332fb90..c184f01920e2 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -56,6 +56,24 @@ 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];
> +}
> +
> +static inline void nf_inet_addr_invert(const union nf_inet_addr *src,
> + union nf_inet_addr *dst)
> +{
> + dst->all[0] = ~src->all[0];
> + dst->all[1] = ~src->all[1];
> + dst->all[2] = ~src->all[2];
> + dst->all[3] = ~src->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..8c3046ea2362 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 */
> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index 6e391308431d..1f658b4546b6 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)))
>
> +#ifdef IP_SET_HASH_WITH_NETMASK
> +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
> @@ -308,6 +319,7 @@ struct htype {
> u8 bucketsize; /* max elements in an array block */
> #ifdef IP_SET_HASH_WITH_NETMASK
> u8 netmask; /* netmask value for subnets to store */
> + union nf_inet_addr bitmask; /* stores bitmask */
I'd better see a new IP_SET_HASH_WITH_BITMASK macro. When a set can be
defined either with netmask or bitmap options, then netmask member above
can be kept to store that value for listing and use a new flag member to
tell which option was used (in the case of none, when the default netmask
value should be applied).
However, store ip_set_netmask(netmask) in the bitmap member.
> #endif
> struct list_head ad; /* Resize add|del backlist */
> struct mtype_elem next; /* temporary storage for uadd */
> @@ -484,6 +496,7 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
> a->timeout == b->timeout &&
> #ifdef IP_SET_HASH_WITH_NETMASK
> x->netmask == y->netmask &&
> + nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
> #endif
> #ifdef IP_SET_HASH_WITH_MARKMASK
> x->markmask == y->markmask &&
> @@ -1283,8 +1296,16 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
> nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
> goto nla_put_failure;
> #ifdef IP_SET_HASH_WITH_NETMASK
> - if (h->netmask != HOST_MASK &&
> - nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
> + if (!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;
> + }
> + }
> + 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
> @@ -1448,7 +1469,9 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> #endif
> u8 hbits;
> #ifdef IP_SET_HASH_WITH_NETMASK
> - u8 netmask;
> + int ret = 0;
> + u8 netmask = 0;
> + union nf_inet_addr bitmask = onesmask;
> #endif
> size_t hsize;
> struct htype *h;
> @@ -1489,10 +1512,22 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> 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;
> + }
Why the "netmask == 0" condition was removed?
> + if (tb[IPSET_ATTR_BITMASK]) {
> + 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
You have to explicitly exclude when both the netmask and bitmask options
are specified.
> @@ -1537,6 +1572,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
> spin_lock_init(&t->hregion[i].lock);
> h->maxelem = maxelem;
> #ifdef IP_SET_HASH_WITH_NETMASK
> + 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 258aeb324483..6976b4bcaa96 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 /* bucketsize, initval support */
Fill the comment out with the introduced functionality :-).
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
> @@ -86,7 +87,12 @@ 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);
> +
> + if (h->netmask != HOST_MASK)
> + ip &= ip_set_netmask(h->netmask);
> + else
> + ip &= h->bitmask.ip;
> +
> if (ip == 0)
> return -EINVAL;
If you store the ip_set_netmask(h->netmask) in h->bitmask.ip, then there's
no need for the condition and the hot paths are not slowed down.
> @@ -119,7 +125,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> - ip &= ip_set_hostmask(h->netmask);
> + if (h->netmask != HOST_MASK)
> + ip &= ip_set_hostmask(h->netmask);
> + else
> + ip &= ntohl(h->bitmask.ip);
>
> if (ip == 0)
> return -IPSET_ERR_HASH_ELEM;
> @@ -193,12 +202,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)
> {
> @@ -224,6 +227,16 @@ hash_ip6_data_next(struct hash_ip6_elem *next, const struct hash_ip6_elem *e)
> #define IP_SET_EMIT_CREATE
> #include "ip_set_hash_gen.h"
>
> +static void
> +hash_ip6_netmask(union nf_inet_addr *ip, u8 netmask,
> + const union nf_inet_addr *bitmask)
> +{
> + if (netmask != HOST_MASK)
> + ip6_netmask(ip, netmask);
> + else
> + nf_inet_addr_mask_inplace(ip, bitmask);
> +}
> +
> static int
> hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
> const struct xt_action_param *par,
> @@ -235,7 +248,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);
> + hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
> if (ipv6_addr_any(&e.ip.in6))
> return -EINVAL;
>
> @@ -274,7 +287,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> - hash_ip6_netmask(&e.ip, h->netmask);
> + hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
> if (ipv6_addr_any(&e.ip.in6))
> return -IPSET_ERR_HASH_ELEM;
>
> @@ -301,6 +314,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..8f5379738ffa 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,7 @@ MODULE_ALIAS("ip_set_hash:ip,port");
>
> /* Type specific function prefix */
> #define HTYPE hash_ipport
> +#define IP_SET_HASH_WITH_NETMASK
If you don't want to introduce the netmask option, then don't use this
macro but IP_SET_HASH_WITH_BITMASK.
> /* IPv4 variant */
>
> @@ -92,12 +94,19 @@ 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);
> + if (h->netmask != HOST_MASK)
> + e.ip &= ip_set_netmask(h->netmask);
> + else
> + e.ip &= h->bitmask.ip;
> + if (e.ip == 0)
> + return -EINVAL;
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
>
> @@ -129,6 +138,13 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> + if (h->netmask != HOST_MASK)
> + e.ip &= ip_set_netmask(h->netmask);
> + else
> + 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]) {
> @@ -245,6 +261,16 @@ hash_ipport6_data_next(struct hash_ipport6_elem *next,
> #define IP_SET_EMIT_CREATE
> #include "ip_set_hash_gen.h"
>
> +static void
> +hash_ipport6_netmask(union nf_inet_addr *ip, u8 netmask,
> + const union nf_inet_addr *bitmask)
> +{
> + if (netmask != HOST_MASK)
> + ip6_netmask(ip, netmask);
> + else
> + nf_inet_addr_mask_inplace(ip, bitmask);
> +}
> +
> static int
> hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
> const struct xt_action_param *par,
> @@ -253,12 +279,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);
> + hash_ipport6_netmask(&e.ip, h->netmask, &h->bitmask);
> + if (ipv6_addr_any(&e.ip.in6))
> + return -EINVAL;
> +
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> }
>
> @@ -298,6 +329,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
> if (ret)
> return ret;
>
> + hash_ipport6_netmask(&e.ip, h->netmask, &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 +391,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..a44d71cbc192 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_NETMASK
> #define IPSET_NET_COUNT 2
>
> /* IPv4 variants */
> @@ -153,7 +155,10 @@ 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]);
> + if (h->netmask != HOST_MASK)
> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & ip_set_netmask(h->netmask));
> + else
> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
I really don't see the intention here. We have normal network addresses
which then masked with either another netmask or a bitmask value.
> e.ip[1] &= ip_set_netmask(e.cidr[1]);
>
> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
> @@ -213,7 +218,13 @@ 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]));
> + if (h->netmask != HOST_MASK)
> + e.ip[0] = htonl(ip & ip_set_hostmask(h->netmask)) &
> + ip_set_hostmask(e.cidr[0]);
> + else
> + e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) &
> + ip_set_hostmask(e.cidr[0]));
> +
> e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
> ret = adtfn(set, &e, &ext, &ext, flags);
> return ip_set_enomatch(ret, flags, adt, set) ? -ret :
> @@ -377,6 +388,16 @@ hash_netnet6_data_next(struct hash_netnet6_elem *next,
> #define IP_SET_EMIT_CREATE
> #include "ip_set_hash_gen.h"
>
> +static inline void
> +hash_netnet6_netmask(union nf_inet_addr *ip, u8 netmask,
> + const union nf_inet_addr *bitmask)
> +{
> + if (netmask != HOST_MASK)
> + ip6_netmask(ip, netmask);
> + else
> + nf_inet_addr_mask_inplace(ip, bitmask);
> +}
> +
> static void
> hash_netnet6_init(struct hash_netnet6_elem *e)
> {
> @@ -404,6 +425,10 @@ 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]);
>
> + hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +439,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 +479,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]);
>
> + hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +515,8 @@ 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_NETMASK] = { .type = NLA_U8 },
> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
> },
> .adt_policy = {
> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
> --
> 2.25.1
>
>
Best regards,
Jozsef
-
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] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c
2022-07-12 21:42 ` Jozsef Kadlecsik
@ 2022-07-13 20:07 ` Vishwanath Pai
0 siblings, 0 replies; 10+ messages in thread
From: Vishwanath Pai @ 2022-07-13 20:07 UTC (permalink / raw)
To: Jozsef Kadlecsik; +Cc: pablo, fw, johunt, netfilter-devel
On 7/12/22 17:42, Jozsef Kadlecsik wrote:
> Hi Vishwanath,
>
> On Wed, 29 Jun 2022, 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.
>>
>> Reviewed-by: Joshua Hunt <johunt@akamai.com>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> ---
>> net/netfilter/ipset/ip_set_hash_ip.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/netfilter/ipset/ip_set_hash_ip.c b/net/netfilter/ipset/ip_set_hash_ip.c
>> index dd30c03d5a23..258aeb324483 100644
>> --- a/net/netfilter/ipset/ip_set_hash_ip.c
>> +++ b/net/netfilter/ipset/ip_set_hash_ip.c
>> @@ -120,12 +120,14 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
>> return ret;
>>
>> ip &= ip_set_hostmask(h->netmask);
>> - e.ip = htonl(ip);
>> - if (e.ip == 0)
>> +
>> + if (ip == 0)
>> return -IPSET_ERR_HASH_ELEM;
>>
>> - if (adt == IPSET_TEST)
>> + if (adt == IPSET_TEST) {
>> + e.ip = htonl(ip);
>> return adtfn(set, &e, &ext, &ext, flags);
>> + }
>>
>> ip_to = ip;
>> if (tb[IPSET_ATTR_IP_TO]) {
>> @@ -145,6 +147,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
>> ip_set_mask_from_to(ip, ip_to, cidr);
>> }
>>
>> + e.ip = htonl(ip);
>> + if (e.ip == 0)
>> + return -IPSET_ERR_HASH_ELEM;
>> +
>> hosts = h->netmask == 32 ? 1 : 2 << (32 - h->netmask - 1);
>>
>> /* 64bit division is not allowed on 32bit */
>
> You are right, however the patch can be made much smaller if the e.ip
> conversion is simply moved from
>
> if (retried) {
> ip = ntohl(h->next.ip);
> e.ip = htonl(ip);
> }
>
> into the next for loop as the first instruction. Could you resend
> your patch that way?
>
> Best regards,
> Jozsef
> -
> 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!Ueht58Jfq7I9Q7kUGd0c_P7hXtIX50eqzQrTiCXOlath9rqfr5WF4srmHwNQ06rfNxUsBM7lCp3bew$
> Address : Wigner Research Centre for Physics
> H-1525 Budapest 114, POB. 49, Hungary
Hi Jozsef,
Agreed that would be much simpler. I'll send a v2.
Thanks,
Vishwanath
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types
2022-07-12 21:06 ` [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Jozsef Kadlecsik
@ 2022-07-14 0:08 ` Vishwanath Pai
2022-07-14 6:08 ` Jozsef Kadlecsik
0 siblings, 1 reply; 10+ messages in thread
From: Vishwanath Pai @ 2022-07-14 0:08 UTC (permalink / raw)
To: Jozsef Kadlecsik; +Cc: pablo, fw, johunt, netfilter-devel
On 7/12/22 17:06, Jozsef Kadlecsik wrote:
> Hi Vishwanath,
>
> On Wed, 29 Jun 2022, Vishwanath Pai wrote:
>
>> We fixed a similar problem before in commit 7f4f7dd4417d ("netfilter:
>> ipset: ipset list may return wrong member count for set with timeout").
>> The same issue exists in ip_set_bitmap_gen.h as well.
>
> Could you rework the patch to solve the issue the same way as for the hash
> types (i.e. scanning the set without locking) like in the commit
> 33f08da28324 (netfilter: ipset: Fix "INFO: rcu detected stall in hash_xxx"
> reports)? I know the bitmap types have got a limited size but it'd be
> great if the general method would be the same across the different types.
>
> Best regards,
> Jozsef
>
>> Test case:
>>
>> $ ipset create test bitmap:ip range 10.0.0.0/8 netmask 24 timeout 5
>> $ ipset add test 10.0.0.0
>> $ ipset add test 10.255.255.255
>> $ sleep 5s
>>
>> $ ipset list test
>> Name: test
>> Type: bitmap:ip
>> Revision: 3
>> Header: range 10.0.0.0-10.255.255.255 netmask 24 timeout 5
>> Size in memory: 532568
>> References: 0
>> Number of entries: 2
>> Members:
>>
>> We return "Number of entries: 2" but no members are listed. That is
>> because when we run mtype_head the garbage collector hasn't run yet, but
>> mtype_list checks and cleans up members with expired timeout. To avoid
>> this we can run mtype_expire before printing the number of elements in
>> mytype_head().
>>
>> Reviewed-by: Joshua Hunt <johunt@akamai.com>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> ---
>> net/netfilter/ipset/ip_set_bitmap_gen.h | 46 ++++++++++++++++++-------
>> 1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/netfilter/ipset/ip_set_bitmap_gen.h b/net/netfilter/ipset/ip_set_bitmap_gen.h
>> index 26ab0e9612d8..dd871305bd6e 100644
>> --- a/net/netfilter/ipset/ip_set_bitmap_gen.h
>> +++ b/net/netfilter/ipset/ip_set_bitmap_gen.h
>> @@ -28,6 +28,7 @@
>> #define mtype_del IPSET_TOKEN(MTYPE, _del)
>> #define mtype_list IPSET_TOKEN(MTYPE, _list)
>> #define mtype_gc IPSET_TOKEN(MTYPE, _gc)
>> +#define mtype_expire IPSET_TOKEN(MTYPE, _expire)
>> #define mtype MTYPE
>>
>> #define get_ext(set, map, id) ((map)->extensions + ((set)->dsize * (id)))
>> @@ -88,13 +89,44 @@ mtype_memsize(const struct mtype *map, size_t dsize)
>> map->elements * dsize;
>> }
>>
>> +/* We should grab set->lock before calling this function */
>> +static void
>> +mtype_expire(struct ip_set *set)
>> +{
>> + struct mtype *map = set->data;
>> + void *x;
>> + u32 id;
>> +
>> + for (id = 0; id < map->elements; id++)
>> + if (mtype_gc_test(id, map, set->dsize)) {
>> + x = get_ext(set, map, id);
>> + if (ip_set_timeout_expired(ext_timeout(x, set))) {
>> + clear_bit(id, map->members);
>> + ip_set_ext_destroy(set, x);
>> + set->elements--;
>> + }
>> + }
>> +}
>> +
>> static int
>> mtype_head(struct ip_set *set, struct sk_buff *skb)
>> {
>> const struct mtype *map = set->data;
>> struct nlattr *nested;
>> - size_t memsize = mtype_memsize(map, set->dsize) + set->ext_size;
>> + size_t memsize;
>> +
>> + /* If any members have expired, set->elements will be wrong,
>> + * mytype_expire function will update it with the right count.
>> + * set->elements can still be incorrect in the case of a huge set
>> + * because elements can timeout during set->list().
>> + */
>> + if (SET_WITH_TIMEOUT(set)) {
>> + spin_lock_bh(&set->lock);
>> + mtype_expire(set);
>> + spin_unlock_bh(&set->lock);
>> + }
>>
>> + memsize = mtype_memsize(map, set->dsize) + set->ext_size;
>> nested = nla_nest_start(skb, IPSET_ATTR_DATA);
>> if (!nested)
>> goto nla_put_failure;
>> @@ -266,22 +298,12 @@ mtype_gc(struct timer_list *t)
>> {
>> struct mtype *map = from_timer(map, t, gc);
>> struct ip_set *set = map->set;
>> - void *x;
>> - u32 id;
>>
>> /* We run parallel with other readers (test element)
>> * but adding/deleting new entries is locked out
>> */
>> spin_lock_bh(&set->lock);
>> - for (id = 0; id < map->elements; id++)
>> - if (mtype_gc_test(id, map, set->dsize)) {
>> - x = get_ext(set, map, id);
>> - if (ip_set_timeout_expired(ext_timeout(x, set))) {
>> - clear_bit(id, map->members);
>> - ip_set_ext_destroy(set, x);
>> - set->elements--;
>> - }
>> - }
>> + mtype_expire(set);
>> spin_unlock_bh(&set->lock);
>>
>> map->gc.expires = jiffies + IPSET_GC_PERIOD(set->timeout) * HZ;
>> --
>> 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!WmVWx08Jyxg7z7R4rXMUupSXptToGLCMfZ7GhOy1yP_Ty0YQZFSbPbvlocRcFRBnQqy787ijhdI-ig$
> Address : Wigner Research Centre for Physics
> H-1525 Budapest 114, POB. 49, Hungary
Hi Jozsef,
Sure, I'll give it a try. It might take me a bit longer cause it looks like a
complex set of changes.
Thanks,
Vishwanath
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types
2022-07-14 0:08 ` Vishwanath Pai
@ 2022-07-14 6:08 ` Jozsef Kadlecsik
0 siblings, 0 replies; 10+ messages in thread
From: Jozsef Kadlecsik @ 2022-07-14 6:08 UTC (permalink / raw)
To: Vishwanath Pai; +Cc: pablo, fw, johunt, netfilter-devel
On Wed, 13 Jul 2022, Vishwanath Pai wrote:
> On 7/12/22 17:06, Jozsef Kadlecsik wrote:
> >
> > On Wed, 29 Jun 2022, Vishwanath Pai wrote:
> >
> >> We fixed a similar problem before in commit 7f4f7dd4417d ("netfilter:
> >> ipset: ipset list may return wrong member count for set with timeout").
> >> The same issue exists in ip_set_bitmap_gen.h as well.
> >
> > Could you rework the patch to solve the issue the same way as for the hash
> > types (i.e. scanning the set without locking) like in the commit
> > 33f08da28324 (netfilter: ipset: Fix "INFO: rcu detected stall in hash_xxx"
> > reports)? I know the bitmap types have got a limited size but it'd be
> > great if the general method would be the same across the different types.
>
> Sure, I'll give it a try. It might take me a bit longer cause it looks like a
> complex set of changes.
I meant not the whole commit 33f08da28324, of course there is no need for
region locking at the bitmap types. Just to count non-expired elements
(and extension size) so that it's a reader function and doesn't need
locking.
Best regards,
Jozsef
-
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] 10+ messages in thread
* Re: [PATCH] netfilter: ipset: Add support for new bitmask parameter
2022-07-12 22:45 ` Jozsef Kadlecsik
@ 2022-07-18 15:18 ` Vishwanath Pai
0 siblings, 0 replies; 10+ messages in thread
From: Vishwanath Pai @ 2022-07-18 15:18 UTC (permalink / raw)
To: Jozsef Kadlecsik
Cc: Pablo Neira Ayuso, Florian Westphal, johunt, netfilter-devel
Hi Jozsef,
Thank you so much for the feedback. I have replied inline below.
On 7/12/22 18:45, Jozsef Kadlecsik wrote:
> Hi,
>
> On Wed, 29 Jun 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
>
> In my opinion new stuff should be added to nftables and not to ipset,
> but see my comments below
>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> Signed-off-by: Joshua Hunt <johunt@akamai.com>
>> ---
>> include/linux/netfilter.h | 18 ++++++++
>> include/uapi/linux/netfilter/ipset/ip_set.h | 1 +
>> net/netfilter/ipset/ip_set_hash_gen.h | 48 ++++++++++++++++++---
>> net/netfilter/ipset/ip_set_hash_ip.c | 36 +++++++++++-----
>> net/netfilter/ipset/ip_set_hash_ipport.c | 39 ++++++++++++++++-
>> net/netfilter/ipset/ip_set_hash_netnet.c | 39 +++++++++++++++--
>
> Why these three set types are picked? If you introduce a new feature, add
> it to all possible set types.
The hash:ip type already had netmask, and we designed bitmask to be an extension
of netmask. The other two were picked because of our customer requirements.
>
> What does the bitmask option exactly means for the hash:net* types? It is
> a set level option and not a per entry level one, so the functionality is
> limited.
>
I don't know exactly how our customer makes use of this, but I think the overall
idea is that they want to mask out bits in the middle of the IP block before
taking action on that packet. It looks like the netmask feature on net,net is a
bit redundant because it already accepts a cidr so maybe I'll remove that and
only add bitmask.
> The netmask and bitmap options are excluding ones, so those should be
> handled that way.
>
Yes, agreed. I could not find a way to do this from the userspace, but I can
return an appropriate error from the kernel if both netmask and bitmask are
provided.
> Please add tests to the testsuite to verify the functionality.
>
>> 6 files changed, 160 insertions(+), 21 deletions(-)
>>
Will do.
>> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
>> index c2c6f332fb90..c184f01920e2 100644
>> --- a/include/linux/netfilter.h
>> +++ b/include/linux/netfilter.h
>> @@ -56,6 +56,24 @@ 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];
>> +}
>> +
>> +static inline void nf_inet_addr_invert(const union nf_inet_addr *src,
>> + union nf_inet_addr *dst)
>> +{
>> + dst->all[0] = ~src->all[0];
>> + dst->all[1] = ~src->all[1];
>> + dst->all[2] = ~src->all[2];
>> + dst->all[3] = ~src->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..8c3046ea2362 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 */
>> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
>> index 6e391308431d..1f658b4546b6 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)))
>>
>> +#ifdef IP_SET_HASH_WITH_NETMASK
>> +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
>> @@ -308,6 +319,7 @@ struct htype {
>> u8 bucketsize; /* max elements in an array block */
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> u8 netmask; /* netmask value for subnets to store */
>> + union nf_inet_addr bitmask; /* stores bitmask */
>
> I'd better see a new IP_SET_HASH_WITH_BITMASK macro. When a set can be
> defined either with netmask or bitmap options, then netmask member above
> can be kept to store that value for listing and use a new flag member to
> tell which option was used (in the case of none, when the default netmask
> value should be applied).
>
> However, store ip_set_netmask(netmask) in the bitmap member.
>
Will do, I will use a separate macro for bitmask.
>> #endif
>> struct list_head ad; /* Resize add|del backlist */
>> struct mtype_elem next; /* temporary storage for uadd */
>> @@ -484,6 +496,7 @@ mtype_same_set(const struct ip_set *a, const struct ip_set *b)
>> a->timeout == b->timeout &&
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> x->netmask == y->netmask &&
>> + nf_inet_addr_cmp(&x->bitmask, &y->bitmask) &&
>> #endif
>> #ifdef IP_SET_HASH_WITH_MARKMASK
>> x->markmask == y->markmask &&
>> @@ -1283,8 +1296,16 @@ mtype_head(struct ip_set *set, struct sk_buff *skb)
>> nla_put_net32(skb, IPSET_ATTR_MAXELEM, htonl(h->maxelem)))
>> goto nla_put_failure;
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> - if (h->netmask != HOST_MASK &&
>> - nla_put_u8(skb, IPSET_ATTR_NETMASK, h->netmask))
>> + if (!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;
>> + }
>> + }
>> + 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
>> @@ -1448,7 +1469,9 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> #endif
>> u8 hbits;
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> - u8 netmask;
>> + int ret = 0;
>> + u8 netmask = 0;
>> + union nf_inet_addr bitmask = onesmask;
>> #endif
>> size_t hsize;
>> struct htype *h;
>> @@ -1489,10 +1512,22 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> 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;
>> + }
>
> Why the "netmask == 0" condition was removed?
>
This check is not needed because of the line immediately above the
if(tb[IPSET_ATTR_NETMASK]) block where we assign either 32 or 128 to netmask.
>> + if (tb[IPSET_ATTR_BITMASK]) {
>> + 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
>
> You have to explicitly exclude when both the netmask and bitmask options
> are specified.
>
Agreed, I will make the two parameters mutually exclusive.
>> @@ -1537,6 +1572,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
>> spin_lock_init(&t->hregion[i].lock);
>> h->maxelem = maxelem;
>> #ifdef IP_SET_HASH_WITH_NETMASK
>> + 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 258aeb324483..6976b4bcaa96 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 /* bucketsize, initval support */
>
> Fill the comment out with the introduced functionality :-).
>
Sorry, I overlooked this, will fix it :)
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@netfilter.org>");
>> @@ -86,7 +87,12 @@ 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);
>> +
>> + if (h->netmask != HOST_MASK)
>> + ip &= ip_set_netmask(h->netmask);
>> + else
>> + ip &= h->bitmask.ip;
>> +
>> if (ip == 0)
>> return -EINVAL;
>
> If you store the ip_set_netmask(h->netmask) in h->bitmask.ip, then there's
> no need for the condition and the hot paths are not slowed down.
>
Agreed, that is a good optimization. Will change it.
>> @@ -119,7 +125,10 @@ hash_ip4_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> - ip &= ip_set_hostmask(h->netmask);
>> + if (h->netmask != HOST_MASK)
>> + ip &= ip_set_hostmask(h->netmask);
>> + else
>> + ip &= ntohl(h->bitmask.ip);
>>
>> if (ip == 0)
>> return -IPSET_ERR_HASH_ELEM;
>> @@ -193,12 +202,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)
>> {
>> @@ -224,6 +227,16 @@ hash_ip6_data_next(struct hash_ip6_elem *next, const struct hash_ip6_elem *e)
>> #define IP_SET_EMIT_CREATE
>> #include "ip_set_hash_gen.h"
>>
>> +static void
>> +hash_ip6_netmask(union nf_inet_addr *ip, u8 netmask,
>> + const union nf_inet_addr *bitmask)
>> +{
>> + if (netmask != HOST_MASK)
>> + ip6_netmask(ip, netmask);
>> + else
>> + nf_inet_addr_mask_inplace(ip, bitmask);
>> +}
>> +
>> static int
>> hash_ip6_kadt(struct ip_set *set, const struct sk_buff *skb,
>> const struct xt_action_param *par,
>> @@ -235,7 +248,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);
>> + hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
>> if (ipv6_addr_any(&e.ip.in6))
>> return -EINVAL;
>>
>> @@ -274,7 +287,7 @@ hash_ip6_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> - hash_ip6_netmask(&e.ip, h->netmask);
>> + hash_ip6_netmask(&e.ip, h->netmask, &h->bitmask);
>> if (ipv6_addr_any(&e.ip.in6))
>> return -IPSET_ERR_HASH_ELEM;
>>
>> @@ -301,6 +314,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..8f5379738ffa 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,7 @@ MODULE_ALIAS("ip_set_hash:ip,port");
>>
>> /* Type specific function prefix */
>> #define HTYPE hash_ipport
>> +#define IP_SET_HASH_WITH_NETMASK
>
> If you don't want to introduce the netmask option, then don't use this
> macro but IP_SET_HASH_WITH_BITMASK.
>
I think our customer needs both netmask and bitmask, but I will check.
>> /* IPv4 variant */
>>
>> @@ -92,12 +94,19 @@ 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);
>> + if (h->netmask != HOST_MASK)
>> + e.ip &= ip_set_netmask(h->netmask);
>> + else
>> + e.ip &= h->bitmask.ip;
>> + if (e.ip == 0)
>> + return -EINVAL;
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>>
>> @@ -129,6 +138,13 @@ hash_ipport4_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> + if (h->netmask != HOST_MASK)
>> + e.ip &= ip_set_netmask(h->netmask);
>> + else
>> + 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]) {
>> @@ -245,6 +261,16 @@ hash_ipport6_data_next(struct hash_ipport6_elem *next,
>> #define IP_SET_EMIT_CREATE
>> #include "ip_set_hash_gen.h"
>>
>> +static void
>> +hash_ipport6_netmask(union nf_inet_addr *ip, u8 netmask,
>> + const union nf_inet_addr *bitmask)
>> +{
>> + if (netmask != HOST_MASK)
>> + ip6_netmask(ip, netmask);
>> + else
>> + nf_inet_addr_mask_inplace(ip, bitmask);
>> +}
>> +
>> static int
>> hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
>> const struct xt_action_param *par,
>> @@ -253,12 +279,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);
>> + hash_ipport6_netmask(&e.ip, h->netmask, &h->bitmask);
>> + if (ipv6_addr_any(&e.ip.in6))
>> + return -EINVAL;
>> +
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> }
>>
>> @@ -298,6 +329,10 @@ hash_ipport6_uadt(struct ip_set *set, struct nlattr *tb[],
>> if (ret)
>> return ret;
>>
>> + hash_ipport6_netmask(&e.ip, h->netmask, &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 +391,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..a44d71cbc192 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_NETMASK
>> #define IPSET_NET_COUNT 2
>>
>> /* IPv4 variants */
>> @@ -153,7 +155,10 @@ 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]);
>> + if (h->netmask != HOST_MASK)
>> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & ip_set_netmask(h->netmask));
>> + else
>> + e.ip[0] &= (ip_set_netmask(e.cidr[0]) & h->bitmask.ip);
>
> I really don't see the intention here. We have normal network addresses
> which then masked with either another netmask or a bitmask value.
>
The netmask option seems redundant here, but with the bitmask we're masking out
bits in the middle of the IP.
>> e.ip[1] &= ip_set_netmask(e.cidr[1]);
>>
>> return adtfn(set, &e, &ext, &opt->ext, opt->cmdflags);
>> @@ -213,7 +218,13 @@ 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]));
>> + if (h->netmask != HOST_MASK)
>> + e.ip[0] = htonl(ip & ip_set_hostmask(h->netmask)) &
>> + ip_set_hostmask(e.cidr[0]);
>> + else
>> + e.ip[0] = htonl(ip & ntohl(h->bitmask.ip) &
>> + ip_set_hostmask(e.cidr[0]));
>> +
>> e.ip[1] = htonl(ip2_from & ip_set_hostmask(e.cidr[1]));
>> ret = adtfn(set, &e, &ext, &ext, flags);
>> return ip_set_enomatch(ret, flags, adt, set) ? -ret :
>> @@ -377,6 +388,16 @@ hash_netnet6_data_next(struct hash_netnet6_elem *next,
>> #define IP_SET_EMIT_CREATE
>> #include "ip_set_hash_gen.h"
>>
>> +static inline void
>> +hash_netnet6_netmask(union nf_inet_addr *ip, u8 netmask,
>> + const union nf_inet_addr *bitmask)
>> +{
>> + if (netmask != HOST_MASK)
>> + ip6_netmask(ip, netmask);
>> + else
>> + nf_inet_addr_mask_inplace(ip, bitmask);
>> +}
>> +
>> static void
>> hash_netnet6_init(struct hash_netnet6_elem *e)
>> {
>> @@ -404,6 +425,10 @@ 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]);
>>
>> + hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +439,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 +479,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]);
>>
>> + hash_netnet6_netmask(&e.ip[0], h->netmask, &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 +515,8 @@ 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_NETMASK] = { .type = NLA_U8 },
>> + [IPSET_ATTR_BITMASK] = { .type = NLA_NESTED },
>> },
>> .adt_policy = {
>> [IPSET_ATTR_IP] = { .type = NLA_NESTED },
>> --
>> 2.25.1
>>
>>
>
> Best regards,
> Jozsef
> -
> 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!RpdjKaLxiPULLF4FBpz0cF4sqDUh_OC90cEwFXoLEIRTy0ekM1GM5Upql6uzT0cC8L1VXh9r0LPSkA$
> Address : Wigner Research Centre for Physics
> H-1525 Budapest 114, POB. 49, Hungary
Thanks for the feedback and comments. I will submit a v2.
Thanks,
Vishwanath
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-18 15:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-29 21:21 [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: regression in ip_set_hash_ip.c Vishwanath Pai
2022-07-12 21:42 ` Jozsef Kadlecsik
2022-07-13 20:07 ` Vishwanath Pai
2022-06-29 21:21 ` [PATCH] netfilter: ipset: Add support for new bitmask parameter Vishwanath Pai
2022-07-12 22:45 ` Jozsef Kadlecsik
2022-07-18 15:18 ` Vishwanath Pai
2022-07-12 21:06 ` [PATCH] netfilter: ipset: ipset list may return wrong member count on bitmap types Jozsef Kadlecsik
2022-07-14 0:08 ` Vishwanath Pai
2022-07-14 6:08 ` Jozsef Kadlecsik
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).