[Dnsmasq-discuss] [PATCH] Regression in 2.81 related to support for multiple IPv6 addresses
Petr Menšík
pemensik at redhat.com
Fri May 1 15:13:33 BST 2020
Hi Harald and Geert,
I looked at it and it is quite confusing what it does. Not quite simple
to understand. I think it is not uncommon for dnsmasq code, but this
brings more knobs to make it more complicated.
I tried to rewrite it hopefully with less repetitions and cleaner code.
However when I did this, I have realized current algorihm is quite bad.
When dnsmasq would have 40 client records for dhcp-host, it would have
to walk throught that list about 4*40 times for every DHCP request. It
seems to me smarter sorting of dhcp_configs would allow single or two
passes. Just push the most specific records with client ids and hwaddr
first, then just hostnames.
Anyway, I think some unit test would be needed. Any opinions on my
patches. Have not yet tested them in real network.
Cheers,
Petr
On 4/30/20 11:07 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).
>>
>
> An attempt for a fix, preferring dhcp-host entries with CONFIG_ADDR or
> CONFIG_ADDR6 flag set. With this change I can use these two configs
> successfully:
>
> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2
> or
> dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
>
> (The latter works in 2.80, but fails in 2.81)
>
>
>
> From 13de4c3346462146ce3826a7fc0546fdb5a569b0 Mon Sep 17 00:00:00 2001
> From: Harald Jensås <hjensas at redhat.com>
> Date: Thu, 30 Apr 2020 10:42:07 +0200
> Subject: [PATCH 1/1] Tag filtering in find_config, filter on IP version
>
> Adds the possibility to filter on a config flag when
> looking up configuration.
>
> Update the DHCP and DHCPv6 code to prefer dhcp-host
> entries in config with CONFIG_ADDR/CONFIG_ADDR6 flag
> set.
>
> This fixes a regression in 2.81 where using one dhcp-host
> entry for IPv4 and another for IPv6 no longer work.
>
> Related: rhbz#1829448
> ---
> src/dhcp-common.c | 48 +++++++++++++++++++++++++++++++++++++----------
> src/dnsmasq.h | 3 ++-
> src/lease.c | 2 +-
> src/rfc2131.c | 6 +++---
> src/rfc3315.c | 6 +++---
> 5 files changed, 47 insertions(+), 18 deletions(-)
>
> diff --git a/src/dhcp-common.c b/src/dhcp-common.c
> index eae9886..1277fa6 100644
> --- a/src/dhcp-common.c
> +++ b/src/dhcp-common.c
> @@ -192,6 +192,18 @@ int match_netid(struct dhcp_netid *check, struct dhcp_netid *pool, int tagnotnee
> return 1;
> }
>
> +/* Is the config flag set. */
> +int match_flag(unsigned int flags, unsigned int flag, int flagnotneeded)
> +{
> + if (flagnotneeded)
> + return 1;
> +
> + if (flags & flag)
> + return 1;
> +
> + return 0;
> +}
> +
> /* return domain or NULL if none. */
> char *strip_hostname(char *hostname)
> {
> @@ -318,7 +330,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> unsigned char *clid, int clid_len,
> unsigned char *hwaddr, int hw_len,
> int hw_type, char *hostname,
> - struct dhcp_netid *tags, int tag_not_needed)
> + struct dhcp_netid *tags, int tag_not_needed,
> + unsigned int flag, int flag_not_needed)
> {
> int count, new;
> struct dhcp_config *config, *candidate;
> @@ -331,7 +344,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> if (config->clid_len == clid_len &&
> memcmp(config->clid, clid, clid_len) == 0 &&
> is_config_in_context(context, config) &&
> - match_netid(config->filter, tags, tag_not_needed))
> + match_netid(config->filter, tags, tag_not_needed) &&
> + match_flag(config->flags, flag, flag_not_needed))
>
> return config;
>
> @@ -341,7 +355,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> if ((!context || !(context->flags & CONTEXT_V6)) && *clid == 0 && config->clid_len == clid_len-1 &&
> memcmp(config->clid, clid+1, clid_len-1) == 0 &&
> is_config_in_context(context, config) &&
> - match_netid(config->filter, tags, tag_not_needed))
> + match_netid(config->filter, tags, tag_not_needed) &&
> + match_flag(config->flags, flag, flag_not_needed))
> return config;
> }
>
> @@ -350,7 +365,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> for (config = configs; config; config = config->next)
> if (config_has_mac(config, hwaddr, hw_len, hw_type) &&
> is_config_in_context(context, config) &&
> - match_netid(config->filter, tags, tag_not_needed))
> + match_netid(config->filter, tags, tag_not_needed) &&
> + match_flag(config->flags, flag, flag_not_needed))
> return config;
>
> if (hostname && context)
> @@ -358,7 +374,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> if ((config->flags & CONFIG_NAME) &&
> hostname_isequal(config->hostname, hostname) &&
> is_config_in_context(context, config) &&
> - match_netid(config->filter, tags, tag_not_needed))
> + match_netid(config->filter, tags, tag_not_needed) &&
> + match_flag(config->flags, flag, flag_not_needed))
> return config;
>
>
> @@ -368,7 +385,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> /* use match with fewest wildcard octets */
> for (candidate = NULL, count = 0, config = configs; config; config = config->next)
> if (is_config_in_context(context, config) &&
> - match_netid(config->filter, tags, tag_not_needed))
> + match_netid(config->filter, tags, tag_not_needed) &&
> + match_flag(config->flags, flag, flag_not_needed))
> for (conf_addr = config->hwaddr; conf_addr; conf_addr = conf_addr->next)
> if (conf_addr->wildcard_mask != 0 &&
> conf_addr->hwaddr_len == hw_len &&
> @@ -382,17 +400,27 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,
> return candidate;
> }
>
> -/* Find tagged configs first. */
> +/* Find tagged configs with flags first. */
> struct dhcp_config *find_config(struct dhcp_config *configs,
> struct dhcp_context *context,
> unsigned char *clid, int clid_len,
> unsigned char *hwaddr, int hw_len,
> - int hw_type, char *hostname, struct dhcp_netid *tags)
> + int hw_type, char *hostname, struct dhcp_netid *tags, unsigned int flag)
> {
> - struct dhcp_config *ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0);
> + /* Find tagged config with flags */
> + struct dhcp_config *ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0, flag, 0);
> +
> + /* Find tagged config without flags */
> + if (!ret)
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0, flag, 1);
> +
> + /* Find untagged config with flags */
> + if (!ret)
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 0);
>
> + /* Find untagged config without flags */
> if (!ret)
> - ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1);
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 1);
>
> return ret;
> }
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 18c381e..bdb085b 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -1571,7 +1571,8 @@ struct dhcp_config *find_config(struct dhcp_config *configs,
> unsigned char *clid, int clid_len,
> unsigned char *hwaddr, int hw_len,
> int hw_type, char *hostname,
> - struct dhcp_netid *filter);
> + struct dhcp_netid *filter,
> + unsigned int flag);
> int config_has_mac(struct dhcp_config *config, unsigned char *hwaddr, int len, int type);
> #ifdef HAVE_LINUX_NETWORK
> char *whichdevice(void);
> diff --git a/src/lease.c b/src/lease.c
> index 23e6fe0..917f7f3 100644
> --- a/src/lease.c
> +++ b/src/lease.c
> @@ -230,7 +230,7 @@ void lease_update_from_configs(void)
> if (lease->flags & (LEASE_TA | LEASE_NA))
> continue;
> else if ((config = find_config(daemon->dhcp_conf, NULL, lease->clid, lease->clid_len,
> - lease->hwaddr, lease->hwaddr_len, lease->hwaddr_type, NULL, NULL)) &&
> + lease->hwaddr, lease->hwaddr_len, lease->hwaddr_type, NULL, NULL, 0)) &&
> (config->flags & CONFIG_NAME) &&
> (!(config->flags & CONFIG_ADDR) || config->addr.s_addr == lease->addr.s_addr))
> lease_set_hostname(lease, config->hostname, 1, get_domain(lease->addr), NULL);
> diff --git a/src/rfc2131.c b/src/rfc2131.c
> index fc54aab..22c2d63 100644
> --- a/src/rfc2131.c
> +++ b/src/rfc2131.c
> @@ -504,7 +504,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
> mess->op = BOOTREPLY;
>
> config = find_config(daemon->dhcp_conf, context, clid, clid_len,
> - mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid));
> + mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid), CONFIG_ADDR);
>
> /* set "known" tag for known hosts */
> if (config)
> @@ -514,7 +514,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
> netid = &known_id;
> }
> else if (find_config(daemon->dhcp_conf, NULL, clid, clid_len,
> - mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid)))
> + mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid), CONFIG_ADDR))
> {
> known_id.net = "known-othernet";
> known_id.next = netid;
> @@ -781,7 +781,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
> to avoid impersonation by name. */
> struct dhcp_config *new = find_config(daemon->dhcp_conf, context, NULL, 0,
> mess->chaddr, mess->hlen,
> - mess->htype, hostname, run_tag_if(netid));
> + mess->htype, hostname, run_tag_if(netid), CONFIG_ADDR);
> if (new && !have_config(new, CONFIG_CLID) && !new->hwaddr)
> {
> config = new;
> diff --git a/src/rfc3315.c b/src/rfc3315.c
> index b3f0a0a..27b0611 100644
> --- a/src/rfc3315.c
> +++ b/src/rfc3315.c
> @@ -527,7 +527,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_
>
> if (state->clid &&
> (config = find_config(daemon->dhcp_conf, state->context, state->clid, state->clid_len,
> - state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags))) &&
> + state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags), CONFIG_ADDR6)) &&
> have_config(config, CONFIG_NAME))
> {
> state->hostname = config->hostname;
> @@ -547,7 +547,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_
> /* Search again now we have a hostname.
> Only accept configs without CLID here, (it won't match)
> to avoid impersonation by name. */
> - struct dhcp_config *new = find_config(daemon->dhcp_conf, state->context, NULL, 0, NULL, 0, 0, state->hostname, run_tag_if(state->tags));
> + struct dhcp_config *new = find_config(daemon->dhcp_conf, state->context, NULL, 0, NULL, 0, 0, state->hostname, run_tag_if(state->tags), CONFIG_ADDR6);
> if (new && !have_config(new, CONFIG_CLID) && !new->hwaddr)
> config = new;
> }
> @@ -574,7 +574,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_
> }
> else if (state->clid &&
> find_config(daemon->dhcp_conf, NULL, state->clid, state->clid_len,
> - state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags)))
> + state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags), CONFIG_ADDR6))
> {
> known_id.net = "known-othernet";
> known_id.next = state->tags;
>
--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-flags-to-find_config-and-match-address-family.patch
Type: text/x-patch
Size: 9467 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20200501/cc4a56be/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Break-find_config_match-into-more-functions.patch
Type: text/x-patch
Size: 5078 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20200501/cc4a56be/attachment-0003.bin>
More information about the Dnsmasq-discuss
mailing list