<div dir="auto"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 1 May 2020, 16:28 Petr Menšík, <<a href="mailto:pemensik@redhat.com" rel="noreferrer noreferrer" target="_blank">pemensik@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Harald and Geert,<br>
<br>
I looked at it and it is quite confusing what it does. Not quite simple<br>
to understand. I think it is not uncommon for dnsmasq code, but this<br>
brings more knobs to make it more complicated.<br>
<br>
I tried to rewrite it hopefully with less repetitions and cleaner code.<br>
However when I did this, I have realized current algorihm is quite bad.<br>
<br>
When dnsmasq would have 40 client records for dhcp-host, it would have<br>
to walk throught that list about 4*40 times for every DHCP request. It<br>
seems to me smarter sorting of dhcp_configs would allow single or two<br>
passes. Just push the most specific records with client ids and hwaddr<br>
first, then just hostnames.<br>
<br>
Anyway, I think some unit test would be needed. Any opinions on my<br>
patches. Have not yet tested them in real network.<br>
<br>
Cheers,<br>
Petr<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Hi Petr,</div><div dir="auto"><br></div><div dir="auto">I looked at the patches, I think the refactoring looks good.</div><div dir="auto"><br></div><div dir="auto">I am slightly worried about the sorting, I know openstack neutron does sorting to ensure IPv4 records come before records without address (IPv6 stateless). Look at the docstring[1] for details. In the future I would like to use tag:dhcpv6 in neutron instead of sorting, but that is'nt really possible before most distros have a version of dnsmasq with support for tags in host definitions.</div><div dir="auto"><br></div><div dir="auto">Today is a public holiday for me, and I am away from computers. I will do some testing as soon as I am back home.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">[1] <a href="https://opendev.org/openstack/neutron/src/branch/master/neutron/agent/linux/dhcp.py#L572">https://opendev.org/openstack/neutron/src/branch/master/neutron/agent/linux/dhcp.py#L572</a><br></div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">//</div><div dir="auto">Harald</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 4/30/20 11:07 AM, Harald Jensås wrote:<br>
> On Wed, 2020-04-29 at 16:18 +0200, Harald Jensås wrote:<br>
>> Hi,<br>
>><br>
>> We discovered an issue introduced in this commit:<br>
>> 137286e9baecf6a3ba97722ef1b49c851b531810<br>
>><br>
>> Prior to this commit one could have two dhcp-host entries, one for<br>
>> IPv4<br>
>> and another for IPv6, for example:<br>
>><br>
>> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2<br>
>> dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2<br>
>><br>
>> This no longer works. In the above example dhcpv6 client succedes,<br>
>> but<br>
>> the dhcpv4 client get 'no address available'. Swapping the order of<br>
>> the<br>
>> two entries in the config file allow the dhcpv4 client to succeed,<br>
>> but<br>
>> then the dhcpv5 client fails.<br>
>><br>
>> Alternative configurations that do work in 2.81:<br>
>><br>
>> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2<br>
>> dhcp-<br>
>> host=tag:dhcpv6,52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2<br>
>><br>
>> or: <br>
>><br>
>> dhcp-<br>
>> host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2<br>
>><br>
>><br>
>> I'm not sure using two dhcp-host entries was ever intended to work,<br>
>> as<br>
>> the manual page states that both ipv4 and ipv6 address can be defined<br>
>> in a single entry.<br>
>><br>
>> A first tought for possible fix would be to internally set the<br>
>> 'tag:dhcpv6' for any dhcp-host entry with only ipv6 addresse(s).<br>
>><br>
> <br>
> An attempt for a fix, preferring dhcp-host entries with CONFIG_ADDR or<br>
> CONFIG_ADDR6 flag set. With this change I can use these two configs<br>
> successfully:<br>
> <br>
> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2<br>
> or<br>
> dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2<br>
> dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2<br>
> <br>
> (The latter works in 2.80, but fails in 2.81)<br>
> <br>
> <br>
> <br>
> From 13de4c3346462146ce3826a7fc0546fdb5a569b0 Mon Sep 17 00:00:00 2001<br>
> From: Harald Jensås <<a href="mailto:hjensas@redhat.com" rel="noreferrer noreferrer noreferrer" target="_blank">hjensas@redhat.com</a>><br>
> Date: Thu, 30 Apr 2020 10:42:07 +0200<br>
> Subject: [PATCH 1/1] Tag filtering in find_config, filter on IP version<br>
> <br>
> Adds the possibility to filter on a config flag when<br>
> looking up configuration.<br>
> <br>
> Update the DHCP and DHCPv6 code to prefer dhcp-host<br>
> entries in config with CONFIG_ADDR/CONFIG_ADDR6 flag<br>
> set.<br>
> <br>
> This fixes a regression in 2.81 where using one dhcp-host<br>
> entry for IPv4 and another for IPv6 no longer work.<br>
> <br>
> Related: rhbz#1829448<br>
> ---<br>
> src/dhcp-common.c | 48 +++++++++++++++++++++++++++++++++++++----------<br>
> src/dnsmasq.h | 3 ++-<br>
> src/lease.c | 2 +-<br>
> src/rfc2131.c | 6 +++---<br>
> src/rfc3315.c | 6 +++---<br>
> 5 files changed, 47 insertions(+), 18 deletions(-)<br>
> <br>
> diff --git a/src/dhcp-common.c b/src/dhcp-common.c<br>
> index eae9886..1277fa6 100644<br>
> --- a/src/dhcp-common.c<br>
> +++ b/src/dhcp-common.c<br>
> @@ -192,6 +192,18 @@ int match_netid(struct dhcp_netid *check, struct dhcp_netid *pool, int tagnotnee<br>
> return 1;<br>
> }<br>
> <br>
> +/* Is the config flag set. */<br>
> +int match_flag(unsigned int flags, unsigned int flag, int flagnotneeded)<br>
> +{<br>
> + if (flagnotneeded)<br>
> + return 1;<br>
> + <br>
> + if (flags & flag)<br>
> + return 1;<br>
> + <br>
> + return 0;<br>
> +}<br>
> +<br>
> /* return domain or NULL if none. */<br>
> char *strip_hostname(char *hostname)<br>
> {<br>
> @@ -318,7 +330,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> unsigned char *clid, int clid_len,<br>
> unsigned char *hwaddr, int hw_len, <br>
> int hw_type, char *hostname,<br>
> - struct dhcp_netid *tags, int tag_not_needed)<br>
> + struct dhcp_netid *tags, int tag_not_needed,<br>
> + unsigned int flag, int flag_not_needed)<br>
> {<br>
> int count, new;<br>
> struct dhcp_config *config, *candidate; <br>
> @@ -331,7 +344,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> if (config->clid_len == clid_len && <br>
> memcmp(config->clid, clid, clid_len) == 0 &&<br>
> is_config_in_context(context, config) &&<br>
> - match_netid(config->filter, tags, tag_not_needed))<br>
> + match_netid(config->filter, tags, tag_not_needed) &&<br>
> + match_flag(config->flags, flag, flag_not_needed))<br>
> <br>
> return config;<br>
> <br>
> @@ -341,7 +355,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> if ((!context || !(context->flags & CONTEXT_V6)) && *clid == 0 && config->clid_len == clid_len-1 &&<br>
> memcmp(config->clid, clid+1, clid_len-1) == 0 &&<br>
> is_config_in_context(context, config) &&<br>
> - match_netid(config->filter, tags, tag_not_needed))<br>
> + match_netid(config->filter, tags, tag_not_needed) &&<br>
> + match_flag(config->flags, flag, flag_not_needed))<br>
> return config;<br>
> }<br>
> <br>
> @@ -350,7 +365,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> for (config = configs; config; config = config->next)<br>
> if (config_has_mac(config, hwaddr, hw_len, hw_type) &&<br>
> is_config_in_context(context, config) &&<br>
> - match_netid(config->filter, tags, tag_not_needed))<br>
> + match_netid(config->filter, tags, tag_not_needed) &&<br>
> + match_flag(config->flags, flag, flag_not_needed))<br>
> return config;<br>
> <br>
> if (hostname && context)<br>
> @@ -358,7 +374,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> if ((config->flags & CONFIG_NAME) && <br>
> hostname_isequal(config->hostname, hostname) &&<br>
> is_config_in_context(context, config) &&<br>
> - match_netid(config->filter, tags, tag_not_needed))<br>
> + match_netid(config->filter, tags, tag_not_needed) &&<br>
> + match_flag(config->flags, flag, flag_not_needed))<br>
> return config;<br>
> <br>
> <br>
> @@ -368,7 +385,8 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> /* use match with fewest wildcard octets */<br>
> for (candidate = NULL, count = 0, config = configs; config; config = config->next)<br>
> if (is_config_in_context(context, config) &&<br>
> - match_netid(config->filter, tags, tag_not_needed))<br>
> + match_netid(config->filter, tags, tag_not_needed) &&<br>
> + match_flag(config->flags, flag, flag_not_needed))<br>
> for (conf_addr = config->hwaddr; conf_addr; conf_addr = conf_addr->next)<br>
> if (conf_addr->wildcard_mask != 0 &&<br>
> conf_addr->hwaddr_len == hw_len && <br>
> @@ -382,17 +400,27 @@ static struct dhcp_config *find_config_match(struct dhcp_config *configs,<br>
> return candidate;<br>
> }<br>
> <br>
> -/* Find tagged configs first. */<br>
> +/* Find tagged configs with flags first. */<br>
> struct dhcp_config *find_config(struct dhcp_config *configs,<br>
> struct dhcp_context *context,<br>
> unsigned char *clid, int clid_len,<br>
> unsigned char *hwaddr, int hw_len, <br>
> - int hw_type, char *hostname, struct dhcp_netid *tags)<br>
> + int hw_type, char *hostname, struct dhcp_netid *tags, unsigned int flag)<br>
> {<br>
> - struct dhcp_config *ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0);<br>
> + /* Find tagged config with flags */<br>
> + struct dhcp_config *ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0, flag, 0);<br>
> +<br>
> + /* Find tagged config without flags */<br>
> + if (!ret)<br>
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0, flag, 1);<br>
> +<br>
> + /* Find untagged config with flags */<br>
> + if (!ret)<br>
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 0);<br>
> <br>
> + /* Find untagged config without flags */<br>
> if (!ret)<br>
> - ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1);<br>
> + ret = find_config_match(configs, context, clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 1);<br>
> <br>
> return ret;<br>
> }<br>
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h<br>
> index 18c381e..bdb085b 100644<br>
> --- a/src/dnsmasq.h<br>
> +++ b/src/dnsmasq.h<br>
> @@ -1571,7 +1571,8 @@ struct dhcp_config *find_config(struct dhcp_config *configs,<br>
> unsigned char *clid, int clid_len,<br>
> unsigned char *hwaddr, int hw_len, <br>
> int hw_type, char *hostname,<br>
> - struct dhcp_netid *filter);<br>
> + struct dhcp_netid *filter,<br>
> + unsigned int flag);<br>
> int config_has_mac(struct dhcp_config *config, unsigned char *hwaddr, int len, int type);<br>
> #ifdef HAVE_LINUX_NETWORK<br>
> char *whichdevice(void);<br>
> diff --git a/src/lease.c b/src/lease.c<br>
> index 23e6fe0..917f7f3 100644<br>
> --- a/src/lease.c<br>
> +++ b/src/lease.c<br>
> @@ -230,7 +230,7 @@ void lease_update_from_configs(void)<br>
> if (lease->flags & (LEASE_TA | LEASE_NA))<br>
> continue;<br>
> else if ((config = find_config(daemon->dhcp_conf, NULL, lease->clid, lease->clid_len, <br>
> - lease->hwaddr, lease->hwaddr_len, lease->hwaddr_type, NULL, NULL)) && <br>
> + lease->hwaddr, lease->hwaddr_len, lease->hwaddr_type, NULL, NULL, 0)) && <br>
> (config->flags & CONFIG_NAME) &&<br>
> (!(config->flags & CONFIG_ADDR) || config->addr.s_addr == lease->addr.s_addr))<br>
> lease_set_hostname(lease, config->hostname, 1, get_domain(lease->addr), NULL);<br>
> diff --git a/src/rfc2131.c b/src/rfc2131.c<br>
> index fc54aab..22c2d63 100644<br>
> --- a/src/rfc2131.c<br>
> +++ b/src/rfc2131.c<br>
> @@ -504,7 +504,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,<br>
> mess->op = BOOTREPLY;<br>
> <br>
> config = find_config(daemon->dhcp_conf, context, clid, clid_len, <br>
> - mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid));<br>
> + mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid), CONFIG_ADDR);<br>
> <br>
> /* set "known" tag for known hosts */<br>
> if (config)<br>
> @@ -514,7 +514,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,<br>
> netid = &known_id;<br>
> }<br>
> else if (find_config(daemon->dhcp_conf, NULL, clid, clid_len, <br>
> - mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid)))<br>
> + mess->chaddr, mess->hlen, mess->htype, NULL, run_tag_if(netid), CONFIG_ADDR))<br>
> {<br>
> <a href="http://known_id.net" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">known_id.net</a> = "known-othernet";<br>
> known_id.next = netid;<br>
> @@ -781,7 +781,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,<br>
> to avoid impersonation by name. */<br>
> struct dhcp_config *new = find_config(daemon->dhcp_conf, context, NULL, 0,<br>
> mess->chaddr, mess->hlen, <br>
> - mess->htype, hostname, run_tag_if(netid));<br>
> + mess->htype, hostname, run_tag_if(netid), CONFIG_ADDR);<br>
> if (new && !have_config(new, CONFIG_CLID) && !new->hwaddr)<br>
> {<br>
> config = new;<br>
> diff --git a/src/rfc3315.c b/src/rfc3315.c<br>
> index b3f0a0a..27b0611 100644<br>
> --- a/src/rfc3315.c<br>
> +++ b/src/rfc3315.c<br>
> @@ -527,7 +527,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_<br>
> <br>
> if (state->clid &&<br>
> (config = find_config(daemon->dhcp_conf, state->context, state->clid, state->clid_len,<br>
> - state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags))) &&<br>
> + state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags), CONFIG_ADDR6)) &&<br>
> have_config(config, CONFIG_NAME))<br>
> {<br>
> state->hostname = config->hostname;<br>
> @@ -547,7 +547,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_<br>
> /* Search again now we have a hostname. <br>
> Only accept configs without CLID here, (it won't match)<br>
> to avoid impersonation by name. */<br>
> - struct dhcp_config *new = find_config(daemon->dhcp_conf, state->context, NULL, 0, NULL, 0, 0, state->hostname, run_tag_if(state->tags));<br>
> + 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);<br>
> if (new && !have_config(new, CONFIG_CLID) && !new->hwaddr)<br>
> config = new;<br>
> }<br>
> @@ -574,7 +574,7 @@ static int dhcp6_no_relay(struct state *state, int msg_type, void *inbuff, size_<br>
> }<br>
> else if (state->clid &&<br>
> find_config(daemon->dhcp_conf, NULL, state->clid, state->clid_len,<br>
> - state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags)))<br>
> + state->mac, state->mac_len, state->mac_type, NULL, run_tag_if(state->tags), CONFIG_ADDR6))<br>
> {<br>
> <a href="http://known_id.net" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">known_id.net</a> = "known-othernet";<br>
> known_id.next = state->tags;<br>
> <br>
<br>
-- <br>
Petr Menšík<br>
Software Engineer<br>
Red Hat, <a href="http://www.redhat.com/" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">http://www.redhat.com/</a><br>
email: <a href="mailto:pemensik@redhat.com" rel="noreferrer noreferrer noreferrer" target="_blank">pemensik@redhat.com</a><br>
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB<br>
_______________________________________________<br>
Dnsmasq-discuss mailing list<br>
<a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" rel="noreferrer noreferrer noreferrer" target="_blank">Dnsmasq-discuss@lists.thekelleys.org.uk</a><br>
<a href="http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss</a><br>
</blockquote></div></div></div>