autofs-5.1.7 - fix a couple of null cache locking problems From: Ian Kent 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 --- 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);