On Fri, 1 May 2020, 16:28 Petr Menšík, <pemen...@redhat.com> wrote:

> 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
>

Hi Petr,

I looked at the patches, I think the refactoring looks good.

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.

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.


[1]
https://opendev.org/openstack/neutron/src/branch/master/neutron/agent/linux/dhcp.py#L572


//
Harald

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 <hjen...@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: pemen...@redhat.com
> PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to