ipset: small series of fixes

ipset: small series of fixes

[Sergey Popovich]

This series of patches contains fixes to the current kernel
upstream version of ipset.

Sergey Popovich (6):
  netfilter: ipset: Properly calculate extensions offsets and total
    length
  netfilter: ipset: Use first timed out entry when no matching entry
    exists
  netfilter: ipset: Destroy extensions before moving non-last entry
  netfilter: ipset: Reset flags on private copy on set resize
  netfilter: ipset: Fix double free of comment string when adding
    element
  netfilter: ipset: Remove expired entries on set resize

 net/netfilter/ipset/ip_set_core.c     |   18 ++++++++++++------
 net/netfilter/ipset/ip_set_hash_gen.h |   32 +++++++++++++++-----------------
 2 files changed, 27 insertions(+), 23 deletions(-)

-- 
1.7.10.4

netfilter: ipset: Properly calculate extensions offsets and total length

[Sergey Popovich]

Offsets and total length returned by the ip_set_elem_len()
calculated incorrectly as initial set element length (i.e.
len parameter) is used multiple times in offset calculations,
also affecting set element total length.

Use initial set element length as start offset, do not add aligned
extension offset to the offset. Return offset as total length of
the set element.

This reduces memory requirements on per element basic for the
hash:* type of sets.

For example output from 'ipset -terse list test-1' on 64-bit PC,
where test-1 is generated via following script:

  #!/bin/bash

  set_name='test-1'

  ipset create "$set_name" hash:net family inet \
              timeout 10800 counters comment \
              hashsize 65536 maxelem 65536

  declare -i o3 o4
  fmt="add $set_name 192.168.%u.%u\n"

  for ((o3 = 0; o3 < 256; o3++)); do
      for ((o4 = 0; o4 < 256; o4++)); do
          printf "$fmt" $o3 $o4
      done
  done |ipset -exist restore

BEFORE this patch is applied

  # ipset -terse list test-1
  Name: test-1
  Type: hash:net
  Revision: 6
  Header: family inet hashsize 65536 maxelem 65536
timeout 10800 counters comment
  Size in memory: 26348440

and AFTER applying patch

  # ipset -terse list test-1
  Name: test-1
  Type: hash:net
  Revision: 6
  Header: family inet hashsize 65536 maxelem 65536
timeout 10800 counters comment
  Size in memory: 7706392
  References: 0

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index d259da3..fb1f2b4 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -365,7 +365,7 @@ size_t
 ip_set_elem_len(struct ip_set *set, struct nlattr *tb[], size_t len)
 {
 	enum ip_set_ext_id id;
-	size_t offset = 0;
+	size_t offset = len;
 	u32 cadt_flags = 0;
 
 	if (tb[IPSET_ATTR_CADT_FLAGS])
@@ -375,12 +375,12 @@ ip_set_elem_len(struct ip_set *set, struct nlattr *tb[], size_t len)
 	for (id = 0; id < IPSET_EXT_ID_MAX; id++) {
 		if (!add_extension(id, cadt_flags, tb))
 			continue;
-		offset += ALIGN(len + offset, ip_set_extensions[id].align);
+		offset = ALIGN(offset, ip_set_extensions[id].align);
 		set->offset[id] = offset;
 		set->extensions |= ip_set_extensions[id].type;
 		offset += ip_set_extensions[id].len;
 	}
-	return len + offset;
+	return offset;
 }
 EXPORT_SYMBOL_GPL(ip_set_elem_len);
 
-- 
1.7.10.4

Re: netfilter: ipset: Properly calculate extensions offsets and total length

[Jozsef Kadlecsik]

On Mon, 16 Mar 2015, Sergey Popovich wrote:

> Offsets and total length returned by the ip_set_elem_len()
> calculated incorrectly as initial set element length (i.e.
> len parameter) is used multiple times in offset calculations,
> also affecting set element total length.
> 
> Use initial set element length as start offset, do not add aligned
> extension offset to the offset. Return offset as total length of
> the set element.
> 
> This reduces memory requirements on per element basic for the
> hash:* type of sets.
> 
> For example output from 'ipset -terse list test-1' on 64-bit PC,
> where test-1 is generated via following script:
> 
>   #!/bin/bash
> 
>   set_name='test-1'
> 
>   ipset create "$set_name" hash:net family inet \
>               timeout 10800 counters comment \
>               hashsize 65536 maxelem 65536
> 
>   declare -i o3 o4
>   fmt="add $set_name 192.168.%u.%u\n"
> 
>   for ((o3 = 0; o3 < 256; o3++)); do
>       for ((o4 = 0; o4 < 256; o4++)); do
>           printf "$fmt" $o3 $o4
>       done
>   done |ipset -exist restore
> 
> BEFORE this patch is applied
> 
>   # ipset -terse list test-1
>   Name: test-1
>   Type: hash:net
>   Revision: 6
>   Header: family inet hashsize 65536 maxelem 65536
> timeout 10800 counters comment
>   Size in memory: 26348440
> 
> and AFTER applying patch
> 
>   # ipset -terse list test-1
>   Name: test-1
>   Type: hash:net
>   Revision: 6
>   Header: family inet hashsize 65536 maxelem 65536
> timeout 10800 counters comment
>   Size in memory: 7706392
>   References: 0
> 
> Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>

Good catch, you are right. Patch is applied.

Best regards,
Jozsef

> ---
>  net/netfilter/ipset/ip_set_core.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index d259da3..fb1f2b4 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -365,7 +365,7 @@ size_t
>  ip_set_elem_len(struct ip_set *set, struct nlattr *tb[], size_t len)
>  {
>  	enum ip_set_ext_id id;
> -	size_t offset = 0;
> +	size_t offset = len;
>  	u32 cadt_flags = 0;
>  
>  	if (tb[IPSET_ATTR_CADT_FLAGS])
> @@ -375,12 +375,12 @@ ip_set_elem_len(struct ip_set *set, struct nlattr *tb[], size_t len)
>  	for (id = 0; id < IPSET_EXT_ID_MAX; id++) {
>  		if (!add_extension(id, cadt_flags, tb))
>  			continue;
> -		offset += ALIGN(len + offset, ip_set_extensions[id].align);
> +		offset = ALIGN(offset, ip_set_extensions[id].align);
>  		set->offset[id] = offset;
>  		set->extensions |= ip_set_extensions[id].type;
>  		offset += ip_set_extensions[id].len;
>  	}
> -	return len + offset;
> +	return offset;
>  }
>  EXPORT_SYMBOL_GPL(ip_set_elem_len);
>  
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-
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

netfilter: ipset: Use first timed out entry when no matching entry exists

[Sergey Popovich]

In timeout enabled sets, when no matching entry is found, use
first timed out entry slot.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_hash_gen.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 974ff38..c55bbbf 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -654,7 +654,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 		/* Reuse first timed out entry */
 		if (SET_WITH_TIMEOUT(set) &&
 		    ip_set_timeout_expired(ext_timeout(data, set)) &&
-		    j != AHASH_MAX(h) + 1)
+		    j == AHASH_MAX(h) + 1)
 			j = i;
 	}
 	if (h->elements >= h->maxelem && SET_WITH_FORCEADD(set) && n->pos) {
-- 
1.7.10.4

netfilter: ipset: Destroy extensions before moving non-last entry

[Sergey Popovich]

If set created with comment extension we should destroy it
prior to reusing removed entry slot. Overwise we freeing
valid entry comment string and leaking one in removed entry.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_hash_gen.h |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index c55bbbf..7122cd8 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -751,19 +751,18 @@ mtype_del(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 		if (SET_WITH_TIMEOUT(set) &&
 		    ip_set_timeout_expired(ext_timeout(data, set)))
 			goto out;
-		if (i != n->pos - 1)
-			/* Not last one */
-			memcpy(data, ahash_data(n, n->pos - 1, set->dsize),
-			       set->dsize);
-
-		n->pos--;
-		h->elements--;
 #ifdef IP_SET_HASH_WITH_NETS
 		for (j = 0; j < IPSET_NET_COUNT; j++)
 			mtype_del_cidr(h, SCIDR(d->cidr, j), NLEN(set->family),
 				       j);
 #endif
 		ip_set_ext_destroy(set, data);
+		if (i != n->pos - 1)
+			/* Not last one */
+			memcpy(data, ahash_data(n, n->pos - 1, set->dsize),
+			       set->dsize);
+		n->pos--;
+		h->elements--;
 		if (n->pos + AHASH_INIT_SIZE < n->size) {
 			void *tmp = kzalloc((n->size - AHASH_INIT_SIZE)
 					    * set->dsize,
-- 
1.7.10.4

netfilter: ipset: Reset flags on private copy on set resize

[Sergey Popovich]

Set resizing takes read lock on the hash table, so we should
not modify any data in the original hash table, as this would
disrupt concurrent readers.

Furthermore set resizing could fail, leaving original set data
in inconsistent state: flags (nomatch currently) would be
cleared on all, but the last, where reallocation fails, set
elements.

Fix this by taking private copy of original set entry for
sets, where it is necessary.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_hash_gen.h |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 7122cd8..fcf75be 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -541,8 +541,9 @@ mtype_resize(struct ip_set *set, bool retried)
 	u8 htable_bits = orig->htable_bits;
 #ifdef IP_SET_HASH_WITH_NETS
 	u8 flags;
+	u8 _data[set->dsize];
 #endif
-	struct mtype_elem *data;
+	struct mtype_elem *data, *hdata;
 	struct mtype_elem *d;
 	struct hbucket *n, *m;
 	u32 i, j;
@@ -581,15 +582,15 @@ retry:
 		for (j = 0; j < n->pos; j++) {
 			data = ahash_data(n, j, set->dsize);
 #ifdef IP_SET_HASH_WITH_NETS
+			hdata = memcpy(_data, data, set->dsize);
 			flags = 0;
-			mtype_data_reset_flags(data, &flags);
+			mtype_data_reset_flags(hdata, &flags);
+#else
+			hdata = data;
 #endif
-			m = hbucket(t, HKEY(data, h->initval, htable_bits));
+			m = hbucket(t, HKEY(hdata, h->initval, htable_bits));
 			ret = hbucket_elem_add(m, AHASH_MAX(h), set->dsize);
 			if (ret < 0) {
-#ifdef IP_SET_HASH_WITH_NETS
-				mtype_data_reset_flags(data, &flags);
-#endif
 				read_unlock_bh(&set->lock);
 				mtype_ahash_destroy(set, t, false);
 				if (ret == -EAGAIN)
@@ -598,9 +599,6 @@ retry:
 			}
 			d = ahash_data(m, m->pos++, set->dsize);
 			memcpy(d, data, set->dsize);
-#ifdef IP_SET_HASH_WITH_NETS
-			mtype_data_reset_flags(d, &flags);
-#endif
 		}
 	}
 
-- 
1.7.10.4

netfilter: ipset: Fix double free of comment string when adding element

[Sergey Popovich]

When new element added hbucket_elem_add() is called to allocate
new ahash slot when number of elements (n->pos) in bucket reaches
current bucket size (n->size). New ahash slot area initialized
with zeros. In case there is room for element in the current
slot hbucket_elem_add() returns without initializing area
for the new element.

This is ok for the most cases, as we use ip_set_ext_destroy()
to free resources used by the extensions. Currently only comment
extension should be destroyed. Destroying comment also initializes
pointer to string to NULL, so that on slot reuse in mtype_add()
ip_set_init_comment() does not free already freed comment string.

However in case of call sequence mtype_del() -> mtype_add()
for set with comment extension enabled it might happen that
ahash_data() points to the dirty area, where comment extension
data area has it's pointer set to already freed area, causing
ip_set_init_comment() in mtype_add() to double free comment
string.

This happens if mtype_del()/mtype_expire() deletes non-last
element from the slot in bucket (and ip_set_ext_destroy()
properly initializes comment string to NULL), then we use
memcpy() to copy last element in place of the deleted element
and finally decrement first free element position n->pos.

Later if new element added to the bucket it's data area is not
clear, and ip_set_init_comment() in mtype_add() will attempt
to free comment string used by an valid element previously
seen at last position or double free area if that element
already deleted.

In most cases this leads to oops as slab cache structures
gets corrupted.

Test case for this condition is following:

  1. Use current upstream ipset code.
  2. Compile kernel with CONFIG_DEBUG_SLAB=y
  3. Use following script to reproduce condition:

    #!/bin/bash

    ipset -exist create test-1 hash:net family inet comment \
           hashsize 65536 maxelem 65536

    declare -i cmd_add_index=0 cmd_del_index=1
    declare -a cmd_a=(
        [cmd_add_index]="add"
        [$cmd_del_index]="del"
    )
    declare -i cmd_index cmd_a_size=${#cmd_a[@]} o4

    fmt="%s test-1 10.0.10.%u%s\n"

    while :; do
      for ((o4 = 0; o4 < 256; o4++)); do
          cmd_index=$((RANDOM % cmd_a_size))
          [ $cmd_index -eq $cmd_add_index ] &&
              comment=" comment $RANDOM" ||
              comment=
          printf "$fmt" "${cmd_a[$cmd_index]}" $o4 "$comment"
      done
    done |ipset -exist restore

    ipset destroy test-1

An slab debug reports would be generated reporting memory
double free conditions and memory corruptions.

Fix by initializing memory area for new element in mtype_add().

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_hash_gen.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index fcf75be..e886d18 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -695,6 +695,7 @@ reuse_slot:
 			goto out;
 		}
 		data = ahash_data(n, n->pos++, set->dsize);
+		memset(data, 0, set->dsize);
 #ifdef IP_SET_HASH_WITH_NETS
 		for (i = 0; i < IPSET_NET_COUNT; i++)
 			mtype_add_cidr(h, SCIDR(d->cidr, i), NLEN(set->family),
-- 
1.7.10.4

netfilter: ipset: Remove expired entries on set resize

[Sergey Popovich]

If set resize requested try to remove expired entries on
timeout enabled set first and retry adding element.
Try this only once, of adding element fails again perform
actual set resize.

Fix expected behaviour by setting retried variable after
resize routine is called, as setting this to true before
calling resize will always ignore attempt to remove
expired entries from the set.

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ua>
---
 net/netfilter/ipset/ip_set_core.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index fb1f2b4..96e4f2f 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1410,10 +1410,16 @@ call_ad(struct sock *ctnl, struct sk_buff *skb, struct ip_set *set,
 		write_lock_bh(&set->lock);
 		ret = set->variant->uadt(set, tb, adt, &lineno, flags, retried);
 		write_unlock_bh(&set->lock);
+
+		if (ret != -EAGAIN || !set->variant->resize)
+			break;
+
+		ret = set->variant->resize(set, retried);
+		if (ret)
+			break;
+
 		retried = true;
-	} while (ret == -EAGAIN &&
-		 set->variant->resize &&
-		 (ret = set->variant->resize(set, retried)) == 0);
+	} while (1);
 
 	if (!ret || (ret == -IPSET_ERR_EXIST && eexist))
 		return 0;
-- 
1.7.10.4