Dear Simon,
dnsmasq v2.73 added --hostsdir which is an efficient way of re-
loading only parts of the cache. When we tried to use hostsdir
yesterday, we identified three problems. They are described
below. Patches addressing them are attached.
--- ISSUE 1 --- Logging imprecision
Assume you have multiple files in hostsdir, dnsmasq can only log
the directory not the file that was the real source:
dnsmasq: read /home/test/hostsdir/hosts1 - 1 addresses
dnsmasq: read /home/test/hostsdir/hosts2 - 1 addresses
dnsmasq: read /home/test/hostsdir/hosts3 - 1 addresses
dnsmasq: 1 127.0.0.1/34170 query[A] aaa from 127.0.0.1
dnsmasq: 1 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 1 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.1
dnsmasq: 1 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
This happens because the cache entries all use the same index
that is the directory name.
--- ISSUE 2 --- Outdated entries are not removed
When hostsdir re-reads the file, it does not remove outdated
entries. Assume you modify "192.168.1.1 aaa" to "192.168.1.2
aaa", dnsmasq will now serve two A records for "aaa". This may be
considered okay, however, if I add "192.168.1.1 bbb", PTR
requests for this domain will still be replied with "aaa" which
might be completely outdated information.
--- ISSUE 3 --- Ever growing replies under certain situations
When a users uses an editor that creates (temporary) files during
editing (like "sed -i") or uses a script that writes files line
by line (like "echo '' >> file"), they can quickly end up with
strange things like
dnsmasq: 3 127.0.0.1/34170 query[A] aaa from 127.0.0.1
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.1
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
dnsmasq: 3 127.0.0.1/34170 /home/test/hostsdir aaa is 192.168.1.2
which is not very meaningful. We check for duplicates before
inserting into the cache, however, duplicate checking can be
foiled here: add_hosts_entry() calls cache_find_by_name() only
once (say it returned "192.168.1.1") so the memcmp() on the
address fails and we can add an arbitrary amount of 192.168.1.2
entries.
-----
For addressing issue 1, I added a new struct *dyndir having a
linked list of struct *hostsfile. With this, cache_insert() can
get the correct index. If a file is newly added, we just add a
new *hostsfile entry to the list (index++).
Issue 2 is an easy one as we can selectively clean the cache when
we know the uid to be removed. This can be called before running
read_hostsfile() to insert new stuff. I added MOVE_FROM and
DELETE to inotify_add_watch() so we catch if a file was removed.
In this case, we only remove old entries.
Issue 3 is fixed by adding a loop over cache_find_by_name() in
add_hosts_entry() to check possible multiple records.
Best,
Dominik
From 8d012b975874d71fa39565a1acddcc65a87d27c6 Mon Sep 17 00:00:00 2001
From: Dominik Derigs <dl...@dl6er.de>
Date: Wed, 29 Sep 2021 08:22:05 +0200
Subject: [PATCH 1/3] Extend hostsdir to differentiate between individual files
in dynamic directories
Signed-off-by: DL6ER <dl...@dl6er.de>
---
src/cache.c | 9 +++--
src/dnsmasq.h | 13 ++++++-
src/inotify.c | 101 ++++++++++++++++++++++++++++++++++----------------
src/option.c | 40 ++++++++++++--------
4 files changed, 111 insertions(+), 52 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index e1d17c4..6c8cfa9 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -1837,6 +1837,7 @@ void dump_cache(time_t now)
char *record_source(unsigned int index)
{
struct hostsfile *ah;
+ struct dyndir *dd;
if (index == SRC_CONFIG)
return "config";
@@ -1848,9 +1849,11 @@ char *record_source(unsigned int index)
return ah->fname;
#ifdef HAVE_INOTIFY
- for (ah = daemon->dynamic_dirs; ah; ah = ah->next)
- if (ah->index == index)
- return ah->fname;
+ /* Dynamic directories contain multiple files */
+ for (dd = daemon->dynamic_dirs; dd; dd = dd->next)
+ for (ah = dd->files; ah; ah = ah->next)
+ if (ah->index == index)
+ return ah->fname;
#endif
return "<unknown>";
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index c8a918a..32bca11 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -679,10 +679,17 @@ struct hostsfile {
struct hostsfile *next;
int flags;
char *fname;
+ unsigned int index; /* matches to cache entries for logging */
+};
+
+struct dyndir {
+ struct dyndir *next;
+ struct hostsfile *files;
+ int flags;
+ char *dname;
#ifdef HAVE_INOTIFY
int wd; /* inotify watch descriptor */
#endif
- unsigned int index; /* matches to cache entries for logging */
};
/* packet-dump flags */
@@ -1127,6 +1134,7 @@ extern struct daemon {
u32 umbrella_org;
u32 umbrella_asset;
u8 umbrella_device[8];
+ int host_index;
struct hostsfile *addn_hosts;
struct dhcp_context *dhcp, *dhcp6;
struct ra_interface *ra_interfaces;
@@ -1147,7 +1155,8 @@ extern struct daemon {
int doing_ra, doing_dhcp6;
struct dhcp_netid_list *dhcp_ignore, *dhcp_ignore_names, *dhcp_gen_names;
struct dhcp_netid_list *force_broadcast, *bootp_dynamic;
- struct hostsfile *dhcp_hosts_file, *dhcp_opts_file, *dynamic_dirs;
+ struct hostsfile *dhcp_hosts_file, *dhcp_opts_file;
+ struct dyndir *dynamic_dirs;
int dhcp_max, tftp_max, tftp_mtu;
int dhcp_server_port, dhcp_client_port;
int start_tftp_port, end_tftp_port;
diff --git a/src/inotify.c b/src/inotify.c
index 3a8e375..724b17c 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -133,77 +133,109 @@ void inotify_dnsmasq_init()
}
}
+static struct hostsfile *dyndir_addhosts(struct dyndir *dd, char *path)
+{
+ /* Check if this file is already known in dd->files */
+ struct hostsfile *ah = NULL;
+ for(ah = dd->files; ah; ah = ah->next)
+ if(ah && ah->fname && strcmp(path, ah->fname) == 0)
+ return ah;
+
+ /* Not known, create new hostsfile record for this dyndir */
+ struct hostsfile *newah = NULL;
+ if(!(newah = whine_malloc(sizeof(struct hostsfile))))
+ return NULL;
+
+ /* Add this file to the tip of the linked list */
+ newah->next = dd->files;
+ dd->files = newah;
+
+ /* Copy flags, set index and the full file path */
+ newah->flags = dd->flags;
+ newah->index = daemon->host_index++;
+ newah->fname = strdup(path);
+
+ return newah;
+}
+
/* initialisation for dynamic-dir. Set inotify watch for each directory, and read pre-existing files */
void set_dynamic_inotify(int flag, int total_size, struct crec **rhash, int revhashsz)
{
- struct hostsfile *ah;
-
- for (ah = daemon->dynamic_dirs; ah; ah = ah->next)
+ struct dyndir *dd;
+
+ for (dd = daemon->dynamic_dirs; dd; dd = dd->next)
{
DIR *dir_stream = NULL;
struct dirent *ent;
struct stat buf;
-
- if (!(ah->flags & flag))
+
+ if (!(dd->flags & flag))
continue;
-
- if (stat(ah->fname, &buf) == -1)
+
+ if (stat(dd->dname, &buf) == -1)
{
my_syslog(LOG_ERR, _("bad dynamic directory %s: %s"),
- ah->fname, strerror(errno));
+ dd->dname, strerror(errno));
continue;
}
if (!(S_ISDIR(buf.st_mode)))
{
my_syslog(LOG_ERR, _("bad dynamic directory %s: %s"),
- ah->fname, _("not a directory"));
+ dd->dname, _("not a directory"));
continue;
}
-
- if (!(ah->flags & AH_WD_DONE))
+
+ if (!(dd->flags & AH_WD_DONE))
{
- ah->wd = inotify_add_watch(daemon->inotifyfd, ah->fname, IN_CLOSE_WRITE | IN_MOVED_TO);
- ah->flags |= AH_WD_DONE;
+ dd->wd = inotify_add_watch(daemon->inotifyfd, dd->dname, IN_CLOSE_WRITE | IN_MOVED_TO);
+ dd->flags |= AH_WD_DONE;
}
/* Read contents of dir _after_ calling add_watch, in the hope of avoiding
a race which misses files being added as we start */
- if (ah->wd == -1 || !(dir_stream = opendir(ah->fname)))
+ if (dd->wd == -1 || !(dir_stream = opendir(dd->dname)))
{
my_syslog(LOG_ERR, _("failed to create inotify for %s: %s"),
- ah->fname, strerror(errno));
+ dd->dname, strerror(errno));
continue;
}
while ((ent = readdir(dir_stream)))
{
- size_t lendir = strlen(ah->fname);
+ size_t lendir = strlen(dd->dname);
size_t lenfile = strlen(ent->d_name);
char *path;
-
+
/* ignore emacs backups and dotfiles */
if (lenfile == 0 ||
ent->d_name[lenfile - 1] == '~' ||
(ent->d_name[0] == '#' && ent->d_name[lenfile - 1] == '#') ||
ent->d_name[0] == '.')
continue;
-
+
if ((path = whine_malloc(lendir + lenfile + 2)))
{
- strcpy(path, ah->fname);
+ strcpy(path, dd->dname);
strcat(path, "/");
strcat(path, ent->d_name);
-
+
+ struct hostsfile *ah = dyndir_addhosts(dd, path);
+ if(!ah)
+ {
+ free(path);
+ continue;
+ }
+
/* ignore non-regular files */
if (stat(path, &buf) != -1 && S_ISREG(buf.st_mode))
{
- if (ah->flags & AH_HOSTS)
+ if (dd->flags & AH_HOSTS)
total_size = read_hostsfile(path, ah->index, total_size, rhash, revhashsz);
#ifdef HAVE_DHCP
- else if (ah->flags & (AH_DHCP_HST | AH_DHCP_OPT))
- option_read_dynfile(path, ah->flags);
+ else if (dd->flags & (AH_DHCP_HST | AH_DHCP_OPT))
+ option_read_dynfile(path, dd->flags);
#endif
}
@@ -218,7 +250,7 @@ void set_dynamic_inotify(int flag, int total_size, struct crec **rhash, int revh
int inotify_check(time_t now)
{
int hit = 0;
- struct hostsfile *ah;
+ struct dyndir *dd;
while (1)
{
@@ -249,22 +281,29 @@ int inotify_check(time_t now)
if (res->wd == in->wd && strcmp(res->file, in->name) == 0)
hit = 1;
- for (ah = daemon->dynamic_dirs; ah; ah = ah->next)
- if (ah->wd == in->wd)
+ for (dd = daemon->dynamic_dirs; dd; dd = dd->next)
+ if (dd->wd == in->wd)
{
- size_t lendir = strlen(ah->fname);
+ size_t lendir = strlen(dd->dname);
char *path;
if ((path = whine_malloc(lendir + in->len + 2)))
{
- strcpy(path, ah->fname);
+ strcpy(path, dd->dname);
strcat(path, "/");
strcat(path, in->name);
my_syslog(LOG_INFO, _("inotify, new or changed file %s"), path);
- if (ah->flags & AH_HOSTS)
+ if (dd->flags & AH_HOSTS)
{
+ struct hostsfile *ah = dyndir_addhosts(dd, path);
+ if(!ah)
+ {
+ free(path);
+ continue;
+ }
+
read_hostsfile(path, ah->index, 0, NULL, 0);
#ifdef HAVE_DHCP
if (daemon->dhcp || daemon->doing_dhcp6)
@@ -278,7 +317,7 @@ int inotify_check(time_t now)
#endif
}
#ifdef HAVE_DHCP
- else if (ah->flags & AH_DHCP_HST)
+ else if (dd->flags & AH_DHCP_HST)
{
if (option_read_dynfile(path, AH_DHCP_HST))
{
@@ -289,7 +328,7 @@ int inotify_check(time_t now)
lease_update_dns(1);
}
}
- else if (ah->flags & AH_DHCP_OPT)
+ else if (dd->flags & AH_DHCP_OPT)
option_read_dynfile(path, AH_DHCP_OPT);
#endif
diff --git a/src/option.c b/src/option.c
index 4e533be..0101a9f 100644
--- a/src/option.c
+++ b/src/option.c
@@ -2134,15 +2134,13 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
case LOPT_DHCP_HOST: /* --dhcp-hostsfile */
case LOPT_DHCP_OPTS: /* --dhcp-optsfile */
- case LOPT_DHCP_INOTIFY: /* --dhcp-hostsdir */
- case LOPT_DHOPT_INOTIFY: /* --dhcp-optsdir */
- case LOPT_HOST_INOTIFY: /* --hostsdir */
case 'H': /* --addn-hosts */
{
struct hostsfile *new = opt_malloc(sizeof(struct hostsfile));
- static unsigned int hosts_index = SRC_AH;
+ if(daemon->host_index == 0)
+ daemon->host_index = SRC_AH;
new->fname = opt_string_alloc(arg);
- new->index = hosts_index++;
+ new->index = daemon->host_index++;
new->flags = 0;
if (option == 'H')
{
@@ -2158,21 +2156,31 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
{
new->next = daemon->dhcp_opts_file;
daemon->dhcp_opts_file = new;
- }
- else
- {
- new->next = daemon->dynamic_dirs;
- daemon->dynamic_dirs = new;
- if (option == LOPT_DHCP_INOTIFY)
- new->flags |= AH_DHCP_HST;
- else if (option == LOPT_DHOPT_INOTIFY)
- new->flags |= AH_DHCP_OPT;
- else if (option == LOPT_HOST_INOTIFY)
- new->flags |= AH_HOSTS;
}
break;
}
+
+ case LOPT_DHCP_INOTIFY: /* --dhcp-hostsdir */
+ case LOPT_DHOPT_INOTIFY: /* --dhcp-optsdir */
+ case LOPT_HOST_INOTIFY: /* --hostsdir */
+ {
+ struct dyndir *new = opt_malloc(sizeof(struct dyndir));
+ if(daemon->host_index == 0)
+ daemon->host_index = SRC_AH;
+ new->dname = opt_string_alloc(arg);
+ new->flags = 0;
+ new->next = daemon->dynamic_dirs;
+ daemon->dynamic_dirs = new;
+ if (option == LOPT_DHCP_INOTIFY)
+ new->flags |= AH_DHCP_HST;
+ else if (option == LOPT_DHOPT_INOTIFY)
+ new->flags |= AH_DHCP_OPT;
+ else if (option == LOPT_HOST_INOTIFY)
+ new->flags |= AH_HOSTS;
+
+ break;
+ }
case LOPT_AUTHSERV: /* --auth-server */
comma = split(arg);
--
2.25.1
From 2db96ad7b278a05a0c9ed9845e2bbab854ac1155 Mon Sep 17 00:00:00 2001
From: Dominik Derigs <dl...@dl6er.de>
Date: Wed, 29 Sep 2021 08:29:07 +0200
Subject: [PATCH 2/3] Flush cache records before re-reading a hostsfiles in
dynamic directories to flush outdated information (inotify)
Signed-off-by: DL6ER <dl...@dl6er.de>
---
src/cache.c | 18 ++++++++++++++++++
src/dnsmasq.h | 1 +
src/inotify.c | 20 ++++++++++++++------
3 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index 6c8cfa9..cb97d16 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -383,6 +383,24 @@ static int is_expired(time_t now, struct crec *crecp)
return 1;
}
+/* Remove entries with a given UID from the cache */
+unsigned int cache_remove_uid(const unsigned int uid)
+{
+ int i;
+ unsigned int removed = 0;
+ struct crec *crecp;
+
+ for (i = 0; i < hash_size; i++)
+ for (crecp = hash_table[i]; crecp; crecp = crecp->hash_next)
+ if (crecp->uid == uid)
+ {
+ cache_unlink(crecp);
+ cache_free(crecp);
+ removed++;
+ }
+ return removed;
+}
+
static struct crec *cache_scan_free(char *name, union all_addr *addr, unsigned short class, time_t now,
unsigned int flags, struct crec **target_crec, unsigned int *target_uid)
{
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 32bca11..a0c7c64 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -1270,6 +1270,7 @@ struct crec *cache_find_by_name(struct crec *crecp,
char *name, time_t now, unsigned int prot);
void cache_end_insert(void);
void cache_start_insert(void);
+unsigned int cache_remove_uid(const unsigned int uid);
int cache_recv_insert(time_t now, int fd);
struct crec *cache_insert(char *name, union all_addr *addr, unsigned short class,
time_t now, unsigned long ttl, unsigned int flags);
diff --git a/src/inotify.c b/src/inotify.c
index 724b17c..26f1804 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -189,7 +189,7 @@ void set_dynamic_inotify(int flag, int total_size, struct crec **rhash, int revh
if (!(dd->flags & AH_WD_DONE))
{
- dd->wd = inotify_add_watch(daemon->inotifyfd, dd->dname, IN_CLOSE_WRITE | IN_MOVED_TO);
+ dd->wd = inotify_add_watch(daemon->inotifyfd, dd->dname, IN_CLOSE_WRITE | IN_MOVE | IN_DELETE);
dd->flags |= AH_WD_DONE;
}
@@ -292,8 +292,11 @@ int inotify_check(time_t now)
strcpy(path, dd->dname);
strcat(path, "/");
strcat(path, in->name);
-
- my_syslog(LOG_INFO, _("inotify, new or changed file %s"), path);
+
+ if(in->mask & (IN_CLOSE_WRITE | IN_MOVED_TO))
+ my_syslog(LOG_INFO, _("inotify, new or changed file %s"), path);
+ else
+ my_syslog(LOG_INFO, _("inotify, removed file %s"), path);
if (dd->flags & AH_HOSTS)
{
@@ -304,7 +307,12 @@ int inotify_check(time_t now)
continue;
}
- read_hostsfile(path, ah->index, 0, NULL, 0);
+ const unsigned int removed = cache_remove_uid(ah->index);
+ if(removed > 0)
+ my_syslog(LOG_INFO, _("removed %u old cache records"), removed);
+
+ if(in->mask & (IN_CLOSE_WRITE | IN_MOVED_TO))
+ read_hostsfile(path, ah->index, 0, NULL, 0);
#ifdef HAVE_DHCP
if (daemon->dhcp || daemon->doing_dhcp6)
{
@@ -317,7 +325,7 @@ int inotify_check(time_t now)
#endif
}
#ifdef HAVE_DHCP
- else if (dd->flags & AH_DHCP_HST)
+ else if (dd->flags & AH_DHCP_HST && (in->mask & (IN_CLOSE_WRITE | IN_MOVED_TO)))
{
if (option_read_dynfile(path, AH_DHCP_HST))
{
@@ -328,7 +336,7 @@ int inotify_check(time_t now)
lease_update_dns(1);
}
}
- else if (dd->flags & AH_DHCP_OPT)
+ else if (dd->flags & AH_DHCP_OPT && (in->mask & (IN_CLOSE_WRITE | IN_MOVED_TO)))
option_read_dynfile(path, AH_DHCP_OPT);
#endif
--
2.25.1
From 78eaf397d7322b389ea48959c5a1626fb2f201f9 Mon Sep 17 00:00:00 2001
From: Dominik Derigs <dl...@dl6er.de>
Date: Wed, 29 Sep 2021 08:33:09 +0200
Subject: [PATCH 3/3] Fix duplicate detection in add_hosts_entry() in cases
where multiple cache records exist for a given name
Signed-off-by: DL6ER <dl...@dl6er.de>
---
src/cache.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index cb97d16..0b7dde8 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -1036,16 +1036,17 @@ struct crec *cache_find_by_addr(struct crec *crecp, union all_addr *addr,
static void add_hosts_entry(struct crec *cache, union all_addr *addr, int addrlen,
unsigned int index, struct crec **rhash, int hashsz)
{
- struct crec *lookup = cache_find_by_name(NULL, cache_get_name(cache), 0, cache->flags & (F_IPV4 | F_IPV6));
int i;
unsigned int j;
+ struct crec *lookup = NULL;
/* Remove duplicates in hosts files. */
- if (lookup && (lookup->flags & F_HOSTS) && memcmp(&lookup->addr, addr, addrlen) == 0)
- {
- free(cache);
- return;
- }
+ while((lookup = cache_find_by_name(lookup, cache_get_name(cache), 0, cache->flags & (F_IPV4 | F_IPV6))))
+ if (lookup && (lookup->flags & F_HOSTS) && memcmp(&lookup->addr, addr, addrlen) == 0)
+ {
+ free(cache);
+ return;
+ }
/* Ensure there is only one address -> name mapping (first one trumps)
We do this by steam here, The entries are kept in hash chains, linked
--
2.25.1
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
