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://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
H-1525 Budapest 114, POB. 49, Hungary
