[Dnsmasq-discuss] listen-backlog option to override default (too small) value

[Albert ARIBAUD]

Hi Donatas,

Le Wed, 7 Dec 2016 14:43:22 +0200
Donatas Abraitis <donatas.abraitis at gmail.com> a écrit:

> Hi folks,
> 
> for our case at Hostinger, we have a problem while too much
> TcpListenOverflows:
> [root at us-imm-dns1 ~]# nstat -az | grep TcpExtListenOverflows
> TcpExtListenOverflows           299                0.0
> [root at us-imm-dns1 ~]# ss -ntl sport = :53
> State       Recv-Q Send-Q
> Local
> Address:Port
> Peer Address:Port LISTEN      0
> 5
> *:53
> *:*
> LISTEN      0
> 5
> :::53
> :::*
> 
> probe kernel.function("tcp_check_req")
> {
>         tcphdr = __get_skb_tcphdr($skb);
>         dport = __tcp_skb_dport(tcphdr)
>         if ($sk->sk_ack_backlog > $sk->sk_max_ack_backlog)
>                 printf("listen queue for port(%d): %d/%d\n",
>                                 dport,
>                                 $sk->sk_ack_backlog,
>                                 $sk->sk_max_ack_backlog);
> }
> 
> [root at us-imm-dns1 ~]# staprun overflow.ko
> listen queue for port(53): 13/5
> listen queue for port(53): 13/5
> listen queue for port(53): 14/5
> 
> here is the proposed patch:
> 
> commit fa610cd424b905720832afc8636373bb132f49c1
> Author: Donatas Abraitis <donatas.abraitis at gmail.com>
> Date:   Sun Dec 9 09:58:51 2012 +0200
> 
>     Add `listen-backlog` option to override default 5 (too small)
> 
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 4b55bb5..b717df3 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -980,6 +980,7 @@ extern struct daemon {
>    struct dhcp_netid_list *force_broadcast, *bootp_dynamic;
>    struct hostsfile *dhcp_hosts_file, *dhcp_opts_file, *dynamic_dirs;
>    int dhcp_max, tftp_max, tftp_mtu;
> +  int listen_backlog;
>    int dhcp_server_port, dhcp_client_port;
>    int start_tftp_port, end_tftp_port;
>    unsigned int min_leasetime;
> diff --git a/src/network.c b/src/network.c
> index d87d08f..1e9d188 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -746,7 +746,7 @@ static int make_sock(union mysockaddr *addr, int
> type, int dienow)
> 
>    if (type == SOCK_STREAM)
>      {
> -      if (listen(fd, 5) == -1)
> +      if (listen(fd, daemon->listen_backlog) == -1)
>         goto err;
>      }
>    else if (family == AF_INET)
> diff --git a/src/option.c b/src/option.c
> index d0d9509..220303e 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_BACKLOG       350
> 
>  #ifdef HAVE_GETOPT_LONG
>  static const struct option opts[] =
> @@ -190,6 +191,7 @@ static const struct myoption opts[] =
>      { "domain-suffix", 1, 0, 's' },
>      { "interface", 1, 0, 'i' },
>      { "listen-address", 1, 0, 'a' },
> +    { "listen-backlog", 1, 0, LOPT_BACKLOG },
>      { "local-service", 0, 0, LOPT_LOCAL_SERVICE },
>      { "bogus-priv", 0, 0, 'b' },
>      { "bogus-nxdomain", 1, 0, 'B' },
> @@ -394,6 +396,7 @@ static struct {
>    { 't', ARG_ONE, "<host_name>", gettext_noop("Specify default
> target in an MX record."), NULL },
>    { 'T', ARG_ONE, "<integer>", gettext_noop("Specify time-to-live in
> seconds for replies from /etc/hosts."), NULL },
>    { LOPT_NEGTTL, ARG_ONE, "<integer>", gettext_noop("Specify
> time-to-live in seconds for negative caching."), NULL },
> +  { LOPT_BACKLOG, ARG_ONE, "<integer>", gettext_noop("Set the backlog
> queue limit."), NULL },
>    { LOPT_MAXTTL, ARG_ONE, "<integer>", gettext_noop("Specify
> time-to-live in seconds for maximum TTL to send to clients."), NULL },
>    { LOPT_MAXCTTL, ARG_ONE, "<integer>", gettext_noop("Specify
> time-to-live ceiling for cache."), NULL },
>    { LOPT_MINCTTL, ARG_ONE, "<integer>", gettext_noop("Specify
> time-to-live floor for cache."), NULL },
> @@ -2286,7 +2289,11 @@ static int one_opt(int option, char *arg, char
> *errstr, char *gen_err, int comma
>           ret_err(gen_err); /* error */
>         break;
>        }
> -
> +
> +    case LOPT_BACKLOG: /* --listen-backlog */
> +      if (!atoi_check(arg, &daemon->listen_backlog))
> +        ret_err(gen_err);
> +      break;
>      case 'a':  /* --listen-address */
>      case LOPT_AUTHPEER: /* --auth-peer */
>        do {
> @@ -4517,6 +4524,7 @@ void read_opts(int argc, char **argv, char
> *compile_opts)
>    daemon->cachesize = CACHESIZ;
>    daemon->ftabsize = FTABSIZ;
>    daemon->port = NAMESERVER_PORT;
> +  daemon->listen_backlog = 5;
>    daemon->dhcp_client_port = DHCP_CLIENT_PORT;
>    daemon->dhcp_server_port = DHCP_SERVER_PORT;
>    daemon->default_resolv.is_default = 1;

I am not qualified to determine if your patch is the right solution to
your problem, but FWIW, I find this patch clear enough and I assume you
have tested it :) and that it actually solves the issue for you. The
only two remarks I have are:

- it would be nice to also add a description for the option
  and its rationale to the manpage;

- is there a way for dnsmasq to detect excessive backlog and emit a
  diagnostic message pointing the operator to the existence and use of
  the listen-backlog option, and if so, could you add this to the patch?

Note that I am in no way a maintainer of dnsmasq, so neither my review
nor my questions should be mistaken for an acceptation of the patch --
only Simon can accept patches.

Amicalement,
-- 
Albert.