Hey all,
and here comes the third resubmission of my patches. I do still
believe that they are improvements. Even one year after writing
them, them do still apply cleanly on the master branch.
Best,
Dominik
-------- Forwarded Message --------
From: Dominik Derigs <dl...@dl6er.de>
To: dnsmasq-discuss@lists.thekelleys.org.uk
<dnsmasq-discuss@lists.thekelleys.org.uk>, Simon Kelley
<si...@thekelleys.org.uk>
Subject: Fwd: [PATCH] Addressing hostsdir shortcomings
Date: Sat, 02 Apr 2022 21:32:30 +0200
Dear Simon,
Second resubmission of my patches.
They still apply cleanly to current master.
Best,
Dominik
-------- Forwarded Message --------
From: Dominik Derigs <dl...@dl6er.de>
To: dnsmasq-discuss@lists.thekelleys.org.uk
<dnsmasq-discuss@lists.thekelleys.org.uk>, Simon Kelley
<si...@thekelleys.org.uk>
Subject: [PATCH] Addressing hostsdir shortcomings
Date: Sat, 08 Jan 2022 11:45:32 +0100
Hey 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
[sent earlier as
https://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2021q3/015704.html
,
resubmitting patches rebased on latest master]
From 7873cc3dbfce3edeb534bf4d0a0030894aaa152a 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 store the individual files as sources
for loggin
Signed-off-by: DL6ER <dl...@dl6er.de>
---
src/cache.c | 9 +++--
src/dnsmasq.h | 13 ++++++-
src/inotify.c | 103 ++++++++++++++++++++++++++++++++++----------------
src/option.c | 40 ++++++++++++--------
4 files changed, 111 insertions(+), 54 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index 246c3f2..e86d69b 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -1839,6 +1839,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";
@@ -1850,9 +1851,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 1b00298..c6efb6b 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -681,10 +681,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 */
@@ -1137,6 +1144,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;
@@ -1157,7 +1165,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..5bf794e 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -133,81 +133,111 @@ 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 = 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 | IN_DELETE);
+ 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
}
-
- free(path);
}
}
@@ -218,7 +248,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 +279,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 +315,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 +326,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 7134ee7..4c04156 100644
--- a/src/option.c
+++ b/src/option.c
@@ -2132,15 +2132,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')
{
@@ -2156,21 +2154,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 01bca9a459000801ed7debb3cadbffb1692b938a 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 rereading a hostsfile
(inotify)
Signed-off-by: DL6ER <dl...@dl6er.de>
---
src/cache.c | 18 ++++++++++++++++++
src/dnsmasq.h | 1 +
src/inotify.c | 4 ++++
3 files changed, 23 insertions(+)
diff --git a/src/cache.c b/src/cache.c
index e86d69b..4e32bd6 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 c6efb6b..52ee63f 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -1288,6 +1288,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 5bf794e..ee070f7 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -302,6 +302,10 @@ int inotify_check(time_t now)
continue;
}
+ const unsigned int removed = cache_remove_uid(ah->index);
+ if(removed > 0)
+ my_syslog(LOG_INFO, _("flushed %u outdated entries"), removed);
+
read_hostsfile(path, ah->index, 0, NULL, 0);
#ifdef HAVE_DHCP
if (daemon->dhcp || daemon->doing_dhcp6)
--
2.25.1
From 3b5c475494c60298b50230c7131744809527309f 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() to recognize
if there are multiple cache records for a 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 4e32bd6..87255f9 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
