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@xxxxxxxxxx>
>> Signed-off-by: Vishwanath Pai <vpai@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxx
> 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
