* [PATCH] netfilter: ipset: ipset list may return wrong member count for set with timeout
@ 2017-08-17 5:23 Vishwanath Pai
2017-09-11 19:36 ` Jozsef Kadlecsik
0 siblings, 1 reply; 3+ messages in thread
From: Vishwanath Pai @ 2017-08-17 5:23 UTC (permalink / raw)
To: pablo, kadlec
Cc: johunt, pai.vishwain, vpai, netfilter-devel, coreteam, netdev
Simple testcase:
$ ipset create test hash:ip timeout 5
$ ipset add test 1.2.3.4
$ ipset add test 1.2.2.2
$ sleep 5
$ ipset l
Name: test
Type: hash:ip
Revision: 5
Header: family inet hashsize 1024 maxelem 65536 timeout 5
Size in memory: 296
References: 0
Number of entries: 2
Members:
We return "Number of entries: 2" but no members are listed. That is
because mtype_list runs "ip_set_timeout_expired" and does not list the
expired entries, but set->elements is never upated (until mtype_gc
cleans it up later).
Reviewed-by: Joshua Hunt <johunt@akamai.com>
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
---
net/netfilter/ipset/ip_set_hash_gen.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index f236c0b..ff3d31c 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -1041,12 +1041,22 @@ struct htype {
static int
mtype_head(struct ip_set *set, struct sk_buff *skb)
{
- const struct htype *h = set->data;
+ struct htype *h = set->data;
const struct htable *t;
struct nlattr *nested;
size_t memsize;
u8 htable_bits;
+ /* If any members have expired, set->elements will be wrong
+ * mytype_expire function will update it with the right count.
+ * we do not hold set->lock here, so grab it first.
+ */
+ if (SET_WITH_TIMEOUT(set)) {
+ spin_lock_bh(&set->lock);
+ mtype_expire(set, h);
+ spin_unlock_bh(&set->lock);
+ }
+
rcu_read_lock_bh();
t = rcu_dereference_bh_nfnl(h->table);
memsize = mtype_ahash_memsize(h, t) + set->ext_size;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] netfilter: ipset: ipset list may return wrong member count for set with timeout
2017-08-17 5:23 [PATCH] netfilter: ipset: ipset list may return wrong member count for set with timeout Vishwanath Pai
@ 2017-09-11 19:36 ` Jozsef Kadlecsik
2017-09-11 19:57 ` Vishwanath Pai
0 siblings, 1 reply; 3+ messages in thread
From: Jozsef Kadlecsik @ 2017-09-11 19:36 UTC (permalink / raw)
To: Vishwanath Pai
Cc: Pablo Neira Ayuso, johunt, pai.vishwain, netfilter-devel,
coreteam, netdev
Hi,
Your patch is applied in the ipset git tree and I'm going to push it for
kernel inclusion.
I modified the comment part: the elements counter can still be incorrect
in the case of a huge set, because elements might time out during the
listing.
Thanks for your patience!
Best regards,
Jozsef
On Thu, 17 Aug 2017, Vishwanath Pai wrote:
> Simple testcase:
>
> $ ipset create test hash:ip timeout 5
> $ ipset add test 1.2.3.4
> $ ipset add test 1.2.2.2
> $ sleep 5
>
> $ ipset l
> Name: test
> Type: hash:ip
> Revision: 5
> Header: family inet hashsize 1024 maxelem 65536 timeout 5
> Size in memory: 296
> References: 0
> Number of entries: 2
> Members:
>
> We return "Number of entries: 2" but no members are listed. That is
> because mtype_list runs "ip_set_timeout_expired" and does not list the
> expired entries, but set->elements is never upated (until mtype_gc
> cleans it up later).
>
> Reviewed-by: Joshua Hunt <johunt@akamai.com>
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> ---
> net/netfilter/ipset/ip_set_hash_gen.h | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index f236c0b..ff3d31c 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
> @@ -1041,12 +1041,22 @@ struct htype {
> static int
> mtype_head(struct ip_set *set, struct sk_buff *skb)
> {
> - const struct htype *h = set->data;
> + struct htype *h = set->data;
> const struct htable *t;
> struct nlattr *nested;
> size_t memsize;
> u8 htable_bits;
>
> + /* If any members have expired, set->elements will be wrong
> + * mytype_expire function will update it with the right count.
> + * we do not hold set->lock here, so grab it first.
> + */
> + if (SET_WITH_TIMEOUT(set)) {
> + spin_lock_bh(&set->lock);
> + mtype_expire(set, h);
> + spin_unlock_bh(&set->lock);
> + }
> +
> rcu_read_lock_bh();
> t = rcu_dereference_bh_nfnl(h->table);
> memsize = mtype_ahash_memsize(h, t) + set->ext_size;
> --
> 1.9.1
>
>
-
E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
H-1525 Budapest 114, POB. 49, Hungary
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] netfilter: ipset: ipset list may return wrong member count for set with timeout
2017-09-11 19:36 ` Jozsef Kadlecsik
@ 2017-09-11 19:57 ` Vishwanath Pai
0 siblings, 0 replies; 3+ messages in thread
From: Vishwanath Pai @ 2017-09-11 19:57 UTC (permalink / raw)
To: Jozsef Kadlecsik
Cc: Pablo Neira Ayuso, johunt, pai.vishwain, netfilter-devel,
coreteam, netdev
Hi Jozsef,
Thank you. Yes, that is true, the count can still be incorrect in the
case of a huge set.
Thanks,
Vishwanath
On 09/11/2017 03:36 PM, Jozsef Kadlecsik wrote:
> Hi,
>
> Your patch is applied in the ipset git tree and I'm going to push it for
> kernel inclusion.
>
> I modified the comment part: the elements counter can still be incorrect
> in the case of a huge set, because elements might time out during the
> listing.
>
> Thanks for your patience!
>
> Best regards,
> Jozsef
>
> On Thu, 17 Aug 2017, Vishwanath Pai wrote:
>
>> Simple testcase:
>>
>> $ ipset create test hash:ip timeout 5
>> $ ipset add test 1.2.3.4
>> $ ipset add test 1.2.2.2
>> $ sleep 5
>>
>> $ ipset l
>> Name: test
>> Type: hash:ip
>> Revision: 5
>> Header: family inet hashsize 1024 maxelem 65536 timeout 5
>> Size in memory: 296
>> References: 0
>> Number of entries: 2
>> Members:
>>
>> We return "Number of entries: 2" but no members are listed. That is
>> because mtype_list runs "ip_set_timeout_expired" and does not list the
>> expired entries, but set->elements is never upated (until mtype_gc
>> cleans it up later).
>>
>> Reviewed-by: Joshua Hunt <johunt@akamai.com>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>> ---
>> net/netfilter/ipset/ip_set_hash_gen.h | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
>> index f236c0b..ff3d31c 100644
>> --- a/net/netfilter/ipset/ip_set_hash_gen.h
>> +++ b/net/netfilter/ipset/ip_set_hash_gen.h
>> @@ -1041,12 +1041,22 @@ struct htype {
>> static int
>> mtype_head(struct ip_set *set, struct sk_buff *skb)
>> {
>> - const struct htype *h = set->data;
>> + struct htype *h = set->data;
>> const struct htable *t;
>> struct nlattr *nested;
>> size_t memsize;
>> u8 htable_bits;
>>
>> + /* If any members have expired, set->elements will be wrong
>> + * mytype_expire function will update it with the right count.
>> + * we do not hold set->lock here, so grab it first.
>> + */
>> + if (SET_WITH_TIMEOUT(set)) {
>> + spin_lock_bh(&set->lock);
>> + mtype_expire(set, h);
>> + spin_unlock_bh(&set->lock);
>> + }
>> +
>> rcu_read_lock_bh();
>> t = rcu_dereference_bh_nfnl(h->table);
>> memsize = mtype_ahash_memsize(h, t) + set->ext_size;
>> --
>> 1.9.1
>>
>>
>
> -
> E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
> PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
> Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
> H-1525 Budapest 114, POB. 49, Hungary
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-11 19:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-17 5:23 [PATCH] netfilter: ipset: ipset list may return wrong member count for set with timeout Vishwanath Pai
2017-09-11 19:36 ` Jozsef Kadlecsik
2017-09-11 19:57 ` Vishwanath Pai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.