autofs-5.1.7 - fix a couple of null cache locking problems
From: Ian Kent <raven@themaw.net>
There's no locking used for null cache access in mount_autofs_direct().
And in master_mount_mounts() an entry could be deleted holding the read
lock only.
Also in each of these cases an unnecessary cache_partial_match() is
done.
In do_readmap() the null cache read lock is taken but it is only needed
for a short time in do_readmap_mount() where an entry could be deleted.
Signed-off-by: Ian Kent <raven@themaw.net>
---
CHANGELOG | 1 +
daemon/direct.c | 18 ++++++++++--------
daemon/master.c | 24 +++++++++++++-----------
daemon/state.c | 20 ++++++++++----------
4 files changed, 34 insertions(+), 29 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index e0b285d1..5bb47099 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -28,6 +28,7 @@
- update configure.
- handle innetgr() not present in musl.
- fix missing unlock in sasl_do_kinit_ext_cc().
+- fix a couple of null cache locking problems.
19/10/2021 autofs-5.1.8
- add xdr_exports().
diff --git a/daemon/direct.c b/daemon/direct.c
index cf3f24d7..316ffd78 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -464,8 +464,6 @@ int mount_autofs_direct(struct autofs_point *ap)
pthread_cleanup_push(master_source_lock_cleanup, ap->entry);
master_source_readlock(ap->entry);
nc = ap->entry->master->nc;
- cache_readlock(nc);
- pthread_cleanup_push(cache_lock_cleanup, nc);
map = ap->entry->maps;
while (map) {
time_t timeout;
@@ -484,9 +482,13 @@ int mount_autofs_direct(struct autofs_point *ap)
pthread_cleanup_push(cache_lock_cleanup, mc);
me = cache_enumerate(mc, NULL);
while (me) {
+ cache_writelock(nc);
ne = cache_lookup_distinct(nc, me->key);
if (ne) {
- if (map->master_line < ne->age) {
+ unsigned int ne_age = ne->age;
+
+ cache_unlock(nc);
+ if (map->master_line < ne_age) {
/* TODO: check return, locking me */
do_mount_autofs_direct(ap, me, timeout);
}
@@ -495,13 +497,14 @@ int mount_autofs_direct(struct autofs_point *ap)
}
nested = cache_partial_match(nc, me->key);
- if (nested) {
+ if (!nested)
+ cache_unlock(nc);
+ else {
+ cache_delete(nc, nested->key);
+ cache_unlock(nc);
error(ap->logopt,
"removing invalid nested null entry %s",
nested->key);
- nested = cache_partial_match(nc, me->key);
- if (nested)
- cache_delete(nc, nested->key);
}
/* TODO: check return, locking me */
@@ -513,7 +516,6 @@ int mount_autofs_direct(struct autofs_point *ap)
map = map->next;
}
pthread_cleanup_pop(1);
- pthread_cleanup_pop(1);
return 0;
}
diff --git a/daemon/master.c b/daemon/master.c
index f99359c5..926119e2 100644
--- a/daemon/master.c
+++ b/daemon/master.c
@@ -1128,12 +1128,14 @@ int master_read_master(struct master *master, time_t age)
{
unsigned int logopt = master->logopt;
struct mapent_cache *nc;
+ int ret = 1;
/*
* We need to clear and re-populate the null map entry cache
* before alowing anyone else to use it.
*/
wait_for_lookups_and_lock(master);
+ pthread_cleanup_push(master_mutex_lock_cleanup, NULL);
if (master->nc) {
cache_writelock(master->nc);
nc = master->nc;
@@ -1144,7 +1146,8 @@ int master_read_master(struct master *master, time_t age)
error(logopt,
"failed to init null map cache for %s",
master->name);
- return 0;
+ ret = 0;
+ goto done;
}
cache_writelock(nc);
master->nc = nc;
@@ -1160,18 +1163,18 @@ int master_read_master(struct master *master, time_t age)
master->read_fail = 0;
/* HUP signal sets master->readall == 1 only */
if (!master->readall) {
- master_mutex_unlock();
- return 0;
+ ret = 0;
+ goto done;
} else
master_mount_mounts(master, age);
}
if (__master_list_empty(master))
warn(logopt, "no mounts in table");
+done:
+ pthread_cleanup_pop(1);
- master_mutex_unlock();
-
- return 1;
+ return ret;
}
int master_notify_submount(struct autofs_point *ap, const char *path, enum states state)
@@ -1438,7 +1441,7 @@ int master_mount_mounts(struct master *master, time_t age)
continue;
}
- cache_readlock(nc);
+ cache_writelock(nc);
ne = cache_lookup_distinct(nc, this->path);
/*
* If this path matched a nulled entry the master map entry
@@ -1447,8 +1450,8 @@ int master_mount_mounts(struct master *master, time_t age)
*/
if (ne) {
int lineno = ne->age;
- cache_unlock(nc);
+ cache_unlock(nc);
/* null entry appears after map entry */
if (this->maps->master_line < lineno) {
warn(ap->logopt,
@@ -1471,14 +1474,13 @@ int master_mount_mounts(struct master *master, time_t age)
master_free_mapent(ap->entry);
continue;
}
+
nested = cache_partial_match(nc, this->path);
if (nested) {
error(ap->logopt,
"removing invalid nested null entry %s",
nested->key);
- nested = cache_partial_match(nc, this->path);
- if (nested)
- cache_delete(nc, nested->key);
+ cache_delete(nc, nested->key);
}
cache_unlock(nc);
cont:
diff --git a/daemon/state.c b/daemon/state.c
index 5df05619..4e70a330 100644
--- a/daemon/state.c
+++ b/daemon/state.c
@@ -333,16 +333,20 @@ static int do_readmap_mount(struct autofs_point *ap,
nc = ap->entry->master->nc;
+ cache_writelock(nc);
ne = cache_lookup_distinct(nc, me->key);
- if (!ne) {
+ if (ne)
+ cache_unlock(nc);
+ else {
nested = cache_partial_match(nc, me->key);
- if (nested) {
+ if (!nested)
+ cache_unlock(nc);
+ else {
+ cache_delete(nc, nested->key);
+ cache_unlock(nc);
error(ap->logopt,
"removing invalid nested null entry %s",
nested->key);
- nested = cache_partial_match(nc, me->key);
- if (nested)
- cache_delete(nc, nested->key);
}
}
@@ -421,7 +425,7 @@ static void *do_readmap(void *arg)
{
struct autofs_point *ap;
struct map_source *map;
- struct mapent_cache *nc, *mc;
+ struct mapent_cache *mc;
struct readmap_args *ra;
int status;
time_t now;
@@ -466,9 +470,6 @@ static void *do_readmap(void *arg)
struct mapent *me;
unsigned int append_alarm = !ap->exp_runfreq;
- nc = ap->entry->master->nc;
- cache_readlock(nc);
- pthread_cleanup_push(cache_lock_cleanup, nc);
master_source_readlock(ap->entry);
pthread_cleanup_push(master_source_lock_cleanup, ap->entry);
map = ap->entry->maps;
@@ -506,7 +507,6 @@ restart:
}
pthread_cleanup_pop(1);
- pthread_cleanup_pop(1);
}
pthread_cleanup_pop(1);