As a patch, it looks pretty good.

The main problem I have is that the new option becomes one of the those
annoying things that have to be set to make things work, but have no
other value. There are already quite a few dnsmasq options which are
essentially "--dont-break" and if I can avoid another, I will.

You say that the PXE bug is "if it receives a (proxy) DHCP
reply instantly", is this really only with proxy DHCP? That's
believable: a PXE implementation which gets the proxy reply _before_ the
standard DHCP reply might well get confused and, let's face it, it
doesn't take much to confuse most PXE clients :-(

If that's the case, would it be enough to delay replies by a fixed time
always and only if they're proxy replies? That would have the effect of
making things just work, with needing a "--dont-break" option, and
wouldn't affect the normal use-case at all.

It's a bit more difficult to implement, but not impossible. worst case,
dhcp_reply() needs to return a flag indicating that PXE-proxy reply was
made, and the call to the delay routine made based on that.

Only other quibble is the duplication of all the poll-loop code into
dhcp_delay(). Would it be neater to make one routine handle waiting for
an icmp-ping reply and just waiting?

Cheers,

Simon.


On 20/03/17 18:37, Floris Bos wrote:
> Adds option to delay replying to DHCP packets by one or more seconds.
> This provides a workaround for a PXE boot firmware implementation
> that has a bug causing it to fail if it receives a (proxy) DHCP
> reply instantly.
> 
> On Linux it looks up the exact receive time of the UDP packet with
> the SIOCGSTAMP ioctl to prevent multiple delays if multiple packets
> come in around the same time.
> ---
> 
> Hi,
> 
> I have a need to introduce a delay before dnsmasq replies to DHCP
> packets.
> 
> Currently have the following working code, inspired by similar
> code in the icmp_ping() method.
> But was wondering if this is the right way to do this, or if there is
> perhaps a better method?
> 
> And if functionality like this would be welcome for upstream
> inclusion, or if the use-case is too obscure?
> 
> Yours sincerely,
> 
> Floris Bos
> ---
>  man/dnsmasq.8 |  5 +++++
>  src/dhcp.c    | 15 +++++++++++++++
>  src/dnsmasq.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  src/dnsmasq.h |  3 ++-
>  src/option.c  |  8 ++++++++
>  5 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/man/dnsmasq.8 b/man/dnsmasq.8
> index 059eafc..5030dae 100644
> --- a/man/dnsmasq.8
> +++ b/man/dnsmasq.8
> @@ -1790,6 +1790,11 @@ a router to advertise prefixes but not a route via 
> itself.
>  .B --ra-param=low,60,1200
>  The interface field may include a wildcard.
>  .TP
> +.B --dhcp-reply-delay=<integer>
> +Delays replying to DHCP packets for at least the specified number of seconds.
> +This can be used as workaround for bugs in PXE boot firmware that does not 
> function properly when
> +receiving an instant reply.
> +.TP
>  .B --enable-tftp[=<interface>[,<interface>]]
>  Enable the TFTP server function. This is deliberately limited to that
>  needed to net-boot a client. Only reading is allowed; the tsize and
> diff --git a/src/dhcp.c b/src/dhcp.c
> index 08952c8..bb45ad0 100644
> --- a/src/dhcp.c
> +++ b/src/dhcp.c
> @@ -342,6 +342,21 @@ void dhcp_packet(time_t now, int pxe_fd)
>        if (iov.iov_len == 0)
>       return;
>      }
> +
> +  if (daemon->reply_delay)
> +    {
> +      int starttime = now;
> +#ifdef HAVE_LINUX_NETWORK
> +      struct timeval tv;
> +
> +      /* Use timestamp of the UDP packet received as start time for delay */
> +      if (ioctl(fd, SIOCGSTAMP, &tv) == 0)
> +     {
> +       starttime = tv.tv_sec;
> +     }
> +#endif
> +      delay_dhcp(starttime, daemon->reply_delay);
> +    }
>    
>    msg.msg_name = &dest;
>    msg.msg_namelen = sizeof(dest);
> diff --git a/src/dnsmasq.c b/src/dnsmasq.c
> index d2cc7cc..f2b0a56 100644
> --- a/src/dnsmasq.c
> +++ b/src/dnsmasq.c
> @@ -1858,6 +1858,49 @@ int icmp_ping(struct in_addr addr)
>  
>    return gotreply;
>  }
> +
> +void delay_dhcp(time_t start, int sec)
> +{
> +  /* Delay processing DHCP packets for "sec" seconds counting from "start",
> +     while continuing to service events on the DNS and TFTP sockets */
> +
> +  int rc, timeout_count;
> +  time_t now;
> +
> +  for (now = dnsmasq_time(), timeout_count = 0;
> +       (difftime(now, start) <= (float)sec) && (timeout_count < sec * 4);)
> +    {
> +      poll_reset();
> +      set_dns_listeners(now);
> +      set_log_writer();
> +
> +#ifdef HAVE_DHCP6
> +      if (daemon->doing_ra)
> +     poll_listen(daemon->icmp6fd, POLLIN);
> +#endif
> +
> +      rc = do_poll(250);
> +
> +      if (rc < 0)
> +     continue;
> +      else if (rc == 0)
> +     timeout_count++;
> +
> +      now = dnsmasq_time();
> +
> +      check_log_writer(0);
> +      check_dns_listeners(now);
> +
> +#ifdef HAVE_DHCP6
> +      if (daemon->doing_ra && poll_check(daemon->icmp6fd, POLLIN))
> +     icmp6_packet(now);
> +#endif
> +
> +#ifdef HAVE_TFTP
> +      check_tftp_listeners(now);
> +#endif
> +    }
> +}
>  #endif
>  
>   
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 6b44e53..e14c8b8 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -960,7 +960,7 @@ extern struct daemon {
>    int max_logs;  /* queue limit */
>    int cachesize, ftabsize;
>    int port, query_port, min_port, max_port;
> -  unsigned long local_ttl, neg_ttl, max_ttl, min_cache_ttl, max_cache_ttl, 
> auth_ttl, dhcp_ttl, use_dhcp_ttl;
> +  unsigned long local_ttl, neg_ttl, max_ttl, min_cache_ttl, max_cache_ttl, 
> auth_ttl, dhcp_ttl, use_dhcp_ttl, reply_delay;
>    char *dns_client_id;
>    struct hostsfile *addn_hosts;
>    struct dhcp_context *dhcp, *dhcp6;
> @@ -1341,6 +1341,7 @@ unsigned char *extended_hwaddr(int hwtype, int hwlen, 
> unsigned char *hwaddr,
>  #ifdef HAVE_DHCP
>  int make_icmp_sock(void);
>  int icmp_ping(struct in_addr addr);
> +void delay_dhcp(time_t start, int sec);
>  #endif
>  void queue_event(int event);
>  void send_alarm(time_t event, time_t now);
> diff --git a/src/option.c b/src/option.c
> index 31c8cb9..7f10e4a 100644
> --- a/src/option.c
> +++ b/src/option.c
> @@ -159,6 +159,7 @@ struct myoption {
>  #define LOPT_SCRIPT_ARP    347
>  #define LOPT_DHCPTTL       348
>  #define LOPT_TFTP_MTU      349
> +#define LOPT_REPLY_DELAY   350
>   
>  #ifdef HAVE_GETOPT_LONG
>  static const struct option opts[] =  
> @@ -323,6 +324,7 @@ static const struct myoption opts[] =
>      { "dns-loop-detect", 0, 0, LOPT_LOOP_DETECT },
>      { "script-arp", 0, 0, LOPT_SCRIPT_ARP },
>      { "dhcp-ttl", 1, 0 , LOPT_DHCPTTL },
> +    { "dhcp-reply-delay", 1, 0, LOPT_REPLY_DELAY },
>      { NULL, 0, 0, 0 }
>    };
>  
> @@ -494,6 +496,7 @@ static struct {
>    { LOPT_LOOP_DETECT, OPT_LOOP_DETECT, NULL, gettext_noop("Detect and remove 
> DNS forwarding loops."), NULL },
>    { LOPT_IGNORE_ADDR, ARG_DUP, "<ipaddr>", gettext_noop("Ignore DNS 
> responses containing ipaddr."), NULL }, 
>    { LOPT_DHCPTTL, ARG_ONE, "<ttl>", gettext_noop("Set TTL in DNS responses 
> with DHCP-derived addresses."), NULL }, 
> +  { LOPT_REPLY_DELAY, ARG_ONE, "<integer>", gettext_noop("Delay DHCP replies 
> for at least number of seconds."), NULL },
>    { 0, 0, NULL, NULL, NULL }
>  }; 
>  
> @@ -2623,6 +2626,7 @@ static int one_opt(int option, char *arg, char *errstr, 
> char *gen_err, int comma
>      case LOPT_MAXCTTL: /* --max-cache-ttl */
>      case LOPT_AUTHTTL: /* --auth-ttl */
>      case LOPT_DHCPTTL: /* --dhcp-ttl */
> +    case LOPT_REPLY_DELAY: /* --dhcp-reply-delay */
>        {
>       int ttl;
>       if (!atoi_check(arg, &ttl))
> @@ -2646,6 +2650,10 @@ static int one_opt(int option, char *arg, char 
> *errstr, char *gen_err, int comma
>           daemon->dhcp_ttl = (unsigned long)ttl;
>           daemon->use_dhcp_ttl = 1;
>         }
> +        else if (option == LOPT_REPLY_DELAY)
> +       {
> +         daemon->reply_delay = (unsigned long)ttl;
> +       }
>       else
>         daemon->local_ttl = (unsigned long)ttl;
>       break;
> 

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to