[Dnsmasq-discuss] Regression in 2.81 related to support for multiple IPv6 addresses - [PATCH 1/1] Fix regression in s_config_in_context() method

Petr Menšík pemensik at redhat.com
Thu May 7 15:14:03 BST 2020


Checked this fix with help of my new unit test, it indeed fixes the
issue correctly. With significantly lower CPU usage than previous fixes.

Please skip my previous patches in this thread and original Harald's
patch. This one is one is correct.

Cheers,
Petr

On 5/7/20 1:03 AM, Harald Jensås wrote:
> On Wed, 2020-04-29 at 16:18 +0200, Harald Jensås wrote:
>> Hi,
>>
>> We discovered an issue introduced in this commit:
>> 137286e9baecf6a3ba97722ef1b49c851b531810
>>
>> Prior to this commit one could have two dhcp-host entries, one for
>> IPv4
>> and another for IPv6, for example:
>>
>>  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
>>  dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
>>
>> This no longer works. In the above example dhcpv6 client succedes,
>> but
>> the dhcpv4 client get 'no address available'. Swapping the order of
>> the
>> two entries in the config file allow the dhcpv4 client to succeed,
>> but
>> then the dhcpv5 client fails.
>>
>> Alternative configurations that do work in 2.81:
>>
>>  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
>>  dhcp-
>> host=tag:dhcpv6,52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
>>
>> or: 
>>
>>  dhcp-
>> host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2
>>
>>
>> I'm not sure using two dhcp-host entries was ever intended to work,
>> as
>> the manual page states that both ipv4 and ipv6 address can be defined
>> in a single entry.
>>
>> A first tought for possible fix would be to internally set the
>> 'tag:dhcpv6' for any dhcp-host entry with only ipv6 addresse(s).
>>
>>
>> Regards,
>> Harald
> 
> I know we already have patches, either way I was looking for the root cause of this bug,
> and I believe the issue is the changes in the is_config_in_context() method.
> 
> I've annotated the part of the patch below, and summarized my conclusion below.
> Also a included a small patch which fixes the issue.
> 
> 
> diff --git a/src/dhcp-common.c b/src/dhcp-common.c
> index a03edc9..c434df0 100644
> --- a/src/dhcp-common.c
> +++ b/src/dhcp-common.c
> @@ -271,26 +271,35 @@ static int is_config_in_context(struct dhcp_context *context, struct dhcp_config
>  {
>    if (!context) /* called via find_config() from lease_update_from_configs() */
>      return 1; 
> -
> -  if (!(config->flags & (CONFIG_ADDR | CONFIG_ADDR6)))
> -    return 1;
> ****  Previously any config without address in either family was considered in context.  ****
>    
>  #ifdef HAVE_DHCP6
> -  if ((context->flags & CONTEXT_V6) && (config->flags & CONFIG_WILDCARD))
> -    return 1;
> -#endif
> +  if (context->flags & CONTEXT_V6)
> +    {
> +       struct addrlist *addr_list;
>  
> -  for (; context; context = context->current)
> -#ifdef HAVE_DHCP6
> -    if (context->flags & CONTEXT_V6) 
> -      {
> -       if ((config->flags & CONFIG_ADDR6) && is_same_net6(&config->addr6, &context->start6, context->prefix))
> -         return 1;
> -      }
> -    else 
> +       if (!(config->flags & CONFIG_ADDR6))
> ****  Here it is considered in context if CONFIG_ADDR6 is NOT set, even if CONFIG_ADDR is set, i.e address family is IPv4 ****
> +        return 1;
> +       
> +        for (; context; context = context->current)
> +         for (addr_list = config->addr6; addr_list; addr_list = addr_list->next)
> +           {
> +             if ((addr_list->flags & ADDRLIST_WILDCARD) && context->prefix == 64)
> +               return 1;
> +             
> +             if (is_same_net6(&addr_list->addr.addr6, &context->start6, context->prefix))
> +               return 1;
> +           }
> +    }
> +  else
>  #endif
> -      if ((config->flags & CONFIG_ADDR) && is_same_net(config->addr, context->start, context->netmask))
> +    {
> +      if (!(config->flags & CONFIG_ADDR))
> ****  Here it is considered in context if CONFIG_ADDR is NOT set, even if CONFIG_ADDR6 is set, i.e address family is IPv6  ****
>         return 1;
> +      
> +      for (; context; context = context->current)
> +       if ((config->flags & CONFIG_ADDR) && is_same_net(config->addr, context->start, context->netmask))
> +         return 1;
> +    }
>  
>    return 0;
>  }
> 
> 
> Conclusion:
>  * Pre commit 137286e9baecf6a3ba97722ef1b49c851b531810:
>     - in context     :: if NOT config_address_family set, regardless of context_address_family
>     - in context     :: if context_address_family == config_address_family
>     - NOT in context :: if context_address_family != config_address_family
>  * Post commit 137286e9baecf6a3ba97722ef1b49c851b531810:
>     - in context :: if NOT config_address_family set, regardless of context_address_family
>     - in context :: if context_address_family == config_address_family
>     - in context :: if context_address_family != config_address_family
> 
> 
> 
> From 3d113137fd64cd0723cbecab6a36a75d3ecfb0a6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Harald=20Jens=C3=A5s?= <hjensas at redhat.com>
> Date: Thu, 7 May 2020 00:33:54 +0200
> Subject: [PATCH 1/1] Fix regression in s_config_in_context() method
> 
> Prior to commit 137286e9baecf6a3ba97722ef1b49c851b531810
> a config would not be considered in context if:
> a) it has no address family flags set
> b) it has the address family flag of current context set
> 
> Since above commit config is considered in context if the
> address family is the opposite of current context.
> 
> The result is that a config with two dhcp-host records,
> one for IPv6 and another for IPv4 no longer works, for
> example with the below config the config with the IPv6
> address would be considered in context for a DHCP(v4)
> request.
>  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
>  dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
> 
> This commit restores the previous behavior.
> ---
>  src/dhcp-common.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/dhcp-common.c b/src/dhcp-common.c
> index eae9886..ffc78ca 100644
> --- a/src/dhcp-common.c
> +++ b/src/dhcp-common.c
> @@ -280,14 +280,18 @@ static int is_config_in_context(struct dhcp_context *context, struct dhcp_config
>  {
>    if (!context) /* called via find_config() from lease_update_from_configs() */
>      return 1; 
> -  
> +
> +  /* No address present in config == in context */
> +  if (!(config->flags & (CONFIG_ADDR | CONFIG_ADDR6)))
> +    return 1;
> +
>  #ifdef HAVE_DHCP6
>    if (context->flags & CONTEXT_V6)
>      {
>         struct addrlist *addr_list;
>  
>         if (!(config->flags & CONFIG_ADDR6))
> -	 return 1;
> +	 return 0;
>         
>          for (; context; context = context->current)
>  	  for (addr_list = config->addr6; addr_list; addr_list = addr_list->next)
> @@ -303,7 +307,7 @@ static int is_config_in_context(struct dhcp_context *context, struct dhcp_config
>  #endif
>      {
>        if (!(config->flags & CONFIG_ADDR))
> -	return 1;
> +	return 0;
>        
>        for (; context; context = context->current)
>  	if ((config->flags & CONFIG_ADDR) && is_same_net(config->addr, context->start, context->netmask))
> 

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB




More information about the Dnsmasq-discuss mailing list