netfilter: ipset: regression in ip_set_hash_ip.c

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(-)

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

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