[PATCH 0/9] Patch series for direct map reload bug

[PATCH 0/9] Patch series for direct map reload bug

[Ian Kent]

There have been reports of log messages like:
automount[nnnn]: handle_packet_expire_direct: can't find map entry for (1048647,114082)

These messages are fatal because the direct mount path must be known to
lookup the mount. Historically the autofs kernel communication packet
size is fixed and based on the maximum path name component size used by
indirect mounts but direct mounts have a variable length multi-component
path. To solve this while maintaining compatability with indirect mounts,
an index of (device, inode) tuples is maintained to allow lookup of the
direct mount path so the map entry can be located.

Also it's necessary to understand that direct mount maps are always read
fully in order to mount direct mount triggers for all direct mount map
entries so they must exist so this approach should always work.

So the message above only occurs for direct mounts and the ioctl file
handle used by each direct mount is stored in the map entry so it's
not possibile to respond to the kernel at all if the map entry cache
entry can't be found.

Unfortunately map entry cache cleanup on update is complicated and can
lead to problems which (I believe) is what's happening here.

While I suspect your reproducer is a different case to what I've seen
it probably symtomatic of the underlying problem.

Patches "autofs-5.1.9 - fix devid update on reload" and "autofs-5.1.9
- fix stale direct mount trigger not umounted on expire" are the main
fixes I was able to identify working through the code.

Please test them and let me know how it goes.

Note: The CHANGELOG hunks will conflict so you'll need to compensate
for that.

Ian

Ian Kent (9):
  autofs-5.1.9 - quiet possibly noisy log message
  autofs-5.1.9 - fix devid update on reload
  autofs-5.1.9 - fix cache writelock must be taken in update_map_cache()
  autofs-5.1.9 - fix skip valid map entries on expire cleanup
  autofs-5.1.9 - remove unnecessary call to
    set_direct_mount_tree_catatonic()
  autofs-5.1.9 - remove unnecessary assignment in umount_multi()
  autofs-5.1.9 - fix direct mount trigger umount failure case
  autofs-5.1.9 - refactor do_umount_autofs_direct()
  autofs-5.1.9 - fix stale direct mount trigger not umounted on expire

 CHANGELOG           |   9 +++
 daemon/automount.c  |  35 +++++-------
 daemon/direct.c     | 130 ++++++++++++++++++++++++++------------------
 daemon/lookup.c     |  33 ++++-------
 daemon/state.c      |  17 ++----
 include/automount.h |   2 +-
 lib/mounts.c        |   1 -
 7 files changed, 118 insertions(+), 109 deletions(-)

-- 
2.50.1

[PATCH 1/9] autofs-5.1.9 - quiet possibly noisy log message

[Ian Kent]

The message that logs when a mount is already mounted and only the
timeout will be updated on map re-read can create a lot of noise in
the log for direct mount maps. But this is normal operation and is of
limited value for both direct and indirect maps, so remove it.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG    | 1 +
 lib/mounts.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG b/CHANGELOG
index 2e4f05cb7..37a0d25c5 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -32,6 +32,7 @@
 - fix lock ordering deadlock in expire_cleanup().
 - fix handling of ignored offsets.
 - fix invalidated map entry handling in hosts module.
+- quiet possibly noisy log message.
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/lib/mounts.c b/lib/mounts.c
index ab16252c7..dd295f4b3 100644
--- a/lib/mounts.c
+++ b/lib/mounts.c
@@ -2880,7 +2880,6 @@ static int remount_active_mount(struct autofs_point *ap,
 
 	/* Re-reading the map, set timeout and return */
 	if (ap->state == ST_READMAP) {
-		debug(ap->logopt, "already mounted, update timeout");
 		ops->timeout(ap->logopt, fd, NULL, timeout);
 		ops->close(ap->logopt, fd);
 		return REMOUNT_READ_MAP;
-- 
2.50.1

[PATCH 2/9] autofs-5.1.9 - fix devid update on reload

[Ian Kent]

On map update if there's a direct mount in the master map it may be
associated with multiple maps.

If there's an entry that's been removed but is present in another map
and the removed entry has a real mount associated with it special case
handling is needed.

Currently the function that looks for these newly valid map entries
doesn't do it properly. It's meant to look for the first "valid" map
entry but doesn't check if the entry is valid. So if a valid entry
follows the an invalid entry it isn't found, the invalid entry is
returned instead.

Once this is fixed the handling of a removed direct mount entry that's
covered by a real mount can be done. Basically, the newly valid map
entry needs to take over the mount (which wasn't being done quite right
either) and the removed map entry deleted so that the covering mount can
expire. From this point onward the newly valid map entry will be used.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG           |  1 +
 daemon/lookup.c     | 33 ++++++++++-----------------------
 daemon/state.c      | 17 +++++------------
 include/automount.h |  2 +-
 4 files changed, 17 insertions(+), 36 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 37a0d25c5..e18666399 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -33,6 +33,7 @@
 - fix handling of ignored offsets.
 - fix invalidated map entry handling in hosts module.
 - quiet possibly noisy log message.
+- fix devid update on reload.
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/lookup.c b/daemon/lookup.c
index dc7794806..c7bf76273 100644
--- a/daemon/lookup.c
+++ b/daemon/lookup.c
@@ -1412,37 +1412,20 @@ void lookup_prune_one_cache(struct autofs_point *ap, struct mapent_cache *mc, ti
 		 * mount if it is a direct mount or it's just a stale indirect
 		 * cache entry.
 		 */
-		valid = lookup_source_valid_mapent(ap, key, LKP_DISTINCT);
-		if (valid && valid->mc == mc) {
-			 /*
-			  * We've found a map entry that has been removed from
-			  * the current cache so it isn't really valid. Set the
-			  * mapent negative to prevent further mount requests
-			  * using the cache entry.
-			  */
-			debug(ap->logopt, "removed map entry detected, mark negative");
-			if (valid->mapent) {
-				free(valid->mapent);
-				valid->mapent = NULL;
-			}
+		valid = lookup_source_valid_mapent(ap, key, LKP_DISTINCT, age);
+		if (valid)
 			cache_unlock(valid->mc);
-			valid = NULL;
-		}
-		if (!valid &&
-		    is_mounted(path, MNTS_REAL)) {
+		else if (is_mounted(path, MNTS_REAL)) {
 			debug(ap->logopt, "prune postponed, %s mounted", path);
 			free(key);
 			free(path);
 			me = cache_enumerate(mc, me);
 			continue;
 		}
-		if (valid)
-			cache_unlock(valid->mc);
 
 		me = cache_enumerate(mc, me);
 		if (me)
 			next_key = strdup(me->key);
-
 		cache_unlock(mc);
 
 		cache_writelock(mc);
@@ -1532,7 +1515,7 @@ int lookup_prune_cache(struct autofs_point *ap, time_t age)
 }
 
 /* Return with cache readlock held */
-struct mapent *lookup_source_valid_mapent(struct autofs_point *ap, const char *key, unsigned int type)
+struct mapent *lookup_source_valid_mapent(struct autofs_point *ap, const char *key, unsigned int type, time_t age)
 {
 	struct master_mapent *entry = ap->entry;
 	struct map_source *map;
@@ -1556,8 +1539,12 @@ struct mapent *lookup_source_valid_mapent(struct autofs_point *ap, const char *k
 			me = cache_lookup_distinct(mc, key);
 		else
 			me = cache_lookup(mc, key);
-		if (me)
-			break;
+		if (me) {
+			/* Valid? */
+			if (me->age >= age)
+				break;
+			me = NULL;
+		}
 		cache_unlock(mc);
 		map = map->next;
 	}
diff --git a/daemon/state.c b/daemon/state.c
index dbd22be61..3760fef9e 100644
--- a/daemon/state.c
+++ b/daemon/state.c
@@ -337,17 +337,7 @@ static int do_readmap_mount(struct autofs_point *ap,
 		 * This is becuase of the requirement to continue running with
 		 * an empty cache awaiting a map re-load.
 		 */
-		valid = lookup_source_valid_mapent(ap, me->key, LKP_DISTINCT);
-		if (valid && valid->mc == me->mc) {
-			/*
-			 * We've found a map entry that has been removed from
-			 * the current cache so there is no need to update it.
-			 * The stale entry will be dealt with when we prune the
-			 * cache later.
-			 */
-			cache_unlock(valid->mc);
-			valid = NULL;
-		}
+		valid = lookup_source_valid_mapent(ap, me->key, LKP_DISTINCT, now);
 		if (valid) {
 			struct mapent_cache *vmc = valid->mc;
 			struct ioctl_ops *ops = get_ioctl_ops();
@@ -369,8 +359,11 @@ static int do_readmap_mount(struct autofs_point *ap,
 			/* Take over the mount if there is one */
 			valid->ioctlfd = me->ioctlfd;
 			me->ioctlfd = -1;
+			/* Same path */
+			valid->dev = me->dev;
+			valid->ino = me->ino;
 			/* Set device and inode number of the new mapent */
-			cache_set_ino_index(vmc, me);
+			cache_set_ino_index(vmc, valid);
 			cache_unlock(vmc);
 			/* Set timeout and calculate the expire run frequency */
 			timeout = get_exp_timeout(ap, map);
diff --git a/include/automount.h b/include/automount.h
index 79a375512..955f5928c 100644
--- a/include/automount.h
+++ b/include/automount.h
@@ -279,7 +279,7 @@ int lookup_nss_mount(struct autofs_point *ap, struct map_source *source, const c
 void lookup_close_lookup(struct autofs_point *ap);
 void lookup_prune_one_cache(struct autofs_point *ap, struct mapent_cache *mc, time_t age);
 int lookup_prune_cache(struct autofs_point *ap, time_t age);
-struct mapent *lookup_source_valid_mapent(struct autofs_point *ap, const char *key, unsigned int type);
+struct mapent *lookup_source_valid_mapent(struct autofs_point *ap, const char *key, unsigned int type, time_t age);
 struct mapent *lookup_source_mapent(struct autofs_point *ap, const char *key, unsigned int type);
 int lookup_source_close_ioctlfd(struct autofs_point *ap, const char *key);
 
-- 
2.50.1

[PATCH 3/9] autofs-5.1.9 - fix cache writelock must be taken in update_map_cache()

[Ian Kent]

In update_map_cache() the cache writelock must be taken because we need
to clean up the map entry as it will be a problem later if we don't.

It's possible that some other process has taken the lock temporarily but
when this function is called the thread does not hold any cache locks so
it should be ok to aquire it.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG          |  1 +
 daemon/automount.c | 12 +++++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index e18666399..d9daf1e2e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -34,6 +34,7 @@
 - fix invalidated map entry handling in hosts module.
 - quiet possibly noisy log message.
 - fix devid update on reload.
+- fix cache writelock must be taken in update_map_cache().
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/automount.c b/daemon/automount.c
index 22994defe..5476e84ba 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -526,13 +526,11 @@ static void update_map_cache(struct autofs_point *ap, const char *path)
 		}
 
 		mc = map->mc;
-		/* If the lock is busy try later */
-		if (cache_try_writelock(mc)) {
-			me = cache_lookup_distinct(mc, key);
-			if (me && me->ioctlfd == -1)
-				cache_delete(mc, key);
-			cache_unlock(mc);
-		}
+		cache_writelock(mc);
+		me = cache_lookup_distinct(mc, key);
+		if (me && me->ioctlfd == -1)
+			cache_delete(mc, key);
+		cache_unlock(mc);
 
 		map = map->next;
 	}
-- 
2.50.1

[PATCH 4/9] autofs-5.1.9 - fix skip valid map entries on expire cleanup

[Ian Kent]

After an expire if the current map entry is stale because it could not
be cleaned up during the map entry prune (due to the presence of a real
mount) it needs to be cleaned up after a successful expire.

Currently this is done by update_map_cache() if the entry is contained
in an invalid map but it should be doing it if the entry is no longer
valid. In addition, if update_map_cache() gets called the umount has
been successful so the ioctlfd does not need to be checked (and is in
fact updated later in the expire process).

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG          | 1 +
 daemon/automount.c | 9 ++-------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index d9daf1e2e..ebdfe32b7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -35,6 +35,7 @@
 - quiet possibly noisy log message.
 - fix devid update on reload.
 - fix cache writelock must be taken in update_map_cache().
+- fix skip valid map entries on expire cleanup.
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/automount.c b/daemon/automount.c
index 5476e84ba..5578a965b 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -519,16 +519,11 @@ static void update_map_cache(struct autofs_point *ap, const char *path)
 	while (map) {
 		struct mapent *me = NULL;
 
-		/* Skip current, in-use cache */
-		if (ap->entry->age <= map->age) {
-			map = map->next;
-			continue;
-		}
-
 		mc = map->mc;
 		cache_writelock(mc);
 		me = cache_lookup_distinct(mc, key);
-		if (me && me->ioctlfd == -1)
+		/* Only for invalid map entries */
+		if (me && map->age > me->age)
 			cache_delete(mc, key);
 		cache_unlock(mc);
 
-- 
2.50.1

[PATCH 5/9] autofs-5.1.9 - remove unnecessary call to set_direct_mount_tree_catatonic()

[Ian Kent]

Function do_umount_autofs_direct() is called in two cases, during a
readmap and when umounting top level direct mounts.

During a readmap, mounts should not be set catatonic so the call to
set_direct_mount_tree_catatonic() is not needed. If it's called for a
top level direct mount the caller, umount_autofs_direct(), calls
set_direct_mount_tree_catatonic() itself already. So remove this
unnecessary call.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG       | 1 +
 daemon/direct.c | 5 +----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index ebdfe32b7..2d312c9fc 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -36,6 +36,7 @@
 - fix devid update on reload.
 - fix cache writelock must be taken in update_map_cache().
 - fix skip valid map entries on expire cleanup.
+- remove unnecessary call to set_direct_mount_tree_catatonic().
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/direct.c b/daemon/direct.c
index 596201e94..dcd69254d 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -186,11 +186,8 @@ int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 			warn(ap->logopt, "mount point %s is in use", me->key);
 			if (ap->state == ST_SHUTDOWN_FORCE)
 				goto force_umount;
-			else {
-				if (ap->state != ST_READMAP)
-					set_direct_mount_tree_catatonic(ap, me);
+			else
 				return 0;
-			}
 			break;
 		case ENOTDIR:
 			error(ap->logopt, "mount point is not a directory");
-- 
2.50.1

[PATCH 6/9] autofs-5.1.9 - remove unnecessary assignment in umount_multi()

[Ian Kent]

Remove the leftover initialisation of left from when there were multiple
calls to umount_subtree_mounts().

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG          | 1 +
 daemon/automount.c | 4 +---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 2d312c9fc..f92d01b07 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -37,6 +37,7 @@
 - fix cache writelock must be taken in update_map_cache().
 - fix skip valid map entries on expire cleanup.
 - remove unnecessary call to set_direct_mount_tree_catatonic().
+- remove unnecessary assignment in umount_multi().
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/automount.c b/daemon/automount.c
index 5578a965b..c2c8d8f00 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -710,9 +710,7 @@ int umount_multi(struct autofs_point *ap, const char *path, int incl)
 		mnts_put_mount(sbmnt);
 	}
 
-	left = 0;
-
-	left += umount_subtree_mounts(ap, path, is_autofs_fs);
+	left = umount_subtree_mounts(ap, path, is_autofs_fs);
 
 	/* Delete detritus like unwanted mountpoints and symlinks */
 	if (left == 0 &&
-- 
2.50.1

[PATCH 7/9] autofs-5.1.9 - fix direct mount trigger umount failure case

[Ian Kent]

In function do_umount_autofs_direct() for the case where the trigger
mount is found to be in use we should be detaching the mount so it gets
umounted but the process can continue using it while it has an open file
handle for it.

This is because direct mount triggers are only umounted when the map
entry is removed from a map or automount(8) is shutdown which n both
cases the trigger mount needs to go away.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG       | 1 +
 daemon/direct.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/CHANGELOG b/CHANGELOG
index f92d01b07..ee1d46987 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -38,6 +38,7 @@
 - fix skip valid map entries on expire cleanup.
 - remove unnecessary call to set_direct_mount_tree_catatonic().
 - remove unnecessary assignment in umount_multi().
+- fix direct mount trigger umount failure case.
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/direct.c b/daemon/direct.c
index dcd69254d..7648b301c 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -153,6 +153,7 @@ int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 			} else {
 				me->ioctlfd = -1;
 				ops->close(ap->logopt, ioctlfd);
+				rv = -1;
 				goto force_umount;
 			}
 		}
-- 
2.50.1

[PATCH 8/9] autofs-5.1.9 - refactor do_umount_autofs_direct()

[Ian Kent]

Refactor functon do_umount_autofs_direct() so that it can be called from
do_expire_direct() to clean up stale direct mounts that couldn't be
cleaned up at map re-load.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG       |   1 +
 daemon/direct.c | 106 ++++++++++++++++++++++++++----------------------
 2 files changed, 58 insertions(+), 49 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index ee1d46987..d41527cc1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -39,6 +39,7 @@
 - remove unnecessary call to set_direct_mount_tree_catatonic().
 - remove unnecessary assignment in umount_multi().
 - fix direct mount trigger umount failure case.
+- refactor do_umount_autofs_direct().
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/direct.c b/daemon/direct.c
index 7648b301c..6562b183a 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -81,12 +81,66 @@ static void mnts_cleanup(void *arg)
 	mnts_put_expire_list(mnts);
 }
 
+static int finish_umount(struct autofs_point *ap, struct mapent *me, int rv)
+{
+	char buf[MAX_ERR_BUF];
+
+	if (rv != 0) {
+		info(ap->logopt, "forcing umount of direct mount %s", me->key);
+		rv = umount2(me->key, MNT_DETACH);
+	} else
+		info(ap->logopt, "umounted direct mount %s", me->key);
+
+	if (!rv && me->flags & MOUNT_FLAG_DIR_CREATED) {
+		if  (rmdir(me->key) == -1) {
+			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
+			warn(ap->logopt, "failed to remove dir %s: %s",
+			     me->key, estr);
+		}
+	}
+	return rv;
+}
+
+static int do_umount_direct(struct autofs_point *ap, struct mapent *me)
+{
+	int rv, retries = UMOUNT_RETRIES;
+
+	while ((rv = umount(me->key)) == -1 && retries--) {
+		struct timespec tm = {0, 50000000};
+		if (errno != EBUSY)
+			break;
+		nanosleep(&tm, NULL);
+	}
+
+	if (rv == -1) {
+		switch (errno) {
+		case ENOENT:
+		case EINVAL:
+			warn(ap->logopt, "mount point %s does not exist",
+			      me->key);
+			return 0;
+		case EBUSY:
+			warn(ap->logopt, "mount point %s is in use", me->key);
+			if (ap->state == ST_SHUTDOWN_FORCE)
+				goto out;
+			else
+				return 0;
+		case ENOTDIR:
+			error(ap->logopt, "mount point is not a directory");
+			return 0;
+		}
+		return 1;
+	}
+out:
+	return finish_umount(ap, me, rv);
+}
+
 int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 {
 	struct ioctl_ops *ops = get_ioctl_ops();
 	struct mapent_cache *mc = me->mc;
 	char buf[MAX_ERR_BUF];
-	int ioctlfd = -1, rv, left, retries;
+	int ioctlfd = -1, rv, left;
 	char key[PATH_MAX + 1];
 	struct mapent *tmp;
 	int opened = 0;
@@ -153,8 +207,7 @@ int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 			} else {
 				me->ioctlfd = -1;
 				ops->close(ap->logopt, ioctlfd);
-				rv = -1;
-				goto force_umount;
+				return finish_umount(ap, me, -1);
 			}
 		}
 		me->ioctlfd = -1;
@@ -167,52 +220,7 @@ int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 
 	sched_yield();
 
-	retries = UMOUNT_RETRIES;
-	while ((rv = umount(me->key)) == -1 && retries--) {
-		struct timespec tm = {0, 50000000};
-		if (errno != EBUSY)
-			break;
-		nanosleep(&tm, NULL);
-	}
-
-	if (rv == -1) {
-		switch (errno) {
-		case ENOENT:
-		case EINVAL:
-			warn(ap->logopt, "mount point %s does not exist",
-			      me->key);
-			return 0;
-			break;
-		case EBUSY:
-			warn(ap->logopt, "mount point %s is in use", me->key);
-			if (ap->state == ST_SHUTDOWN_FORCE)
-				goto force_umount;
-			else
-				return 0;
-			break;
-		case ENOTDIR:
-			error(ap->logopt, "mount point is not a directory");
-			return 0;
-			break;
-		}
-		return 1;
-	}
-
-force_umount:
-	if (rv != 0) {
-		info(ap->logopt, "forcing umount of direct mount %s", me->key);
-		rv = umount2(me->key, MNT_DETACH);
-	} else
-		info(ap->logopt, "umounted direct mount %s", me->key);
-
-	if (!rv && me->flags & MOUNT_FLAG_DIR_CREATED) {
-		if  (rmdir(me->key) == -1) {
-			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
-			warn(ap->logopt, "failed to remove dir %s: %s",
-			     me->key, estr);
-		}
-	}
-	return rv;
+	return do_umount_direct(ap, me);
 }
 
 int umount_autofs_direct(struct autofs_point *ap)
-- 
2.50.1

[PATCH 9/9] autofs-5.1.9 - fix stale direct mount trigger not umounted on expire

[Ian Kent]

If a direct mount map entry is removed but has an active real mount the
mount trigger needs to be unmounted during the expire cleanup.

If the direct mount map entry has been re-added the map entry age will
have been updated so the entry won't be seen as stale so the umount
won't be done.

Also in function umount_multi() update_map_cache() and check_rm_dirs()
are not called for direct mounts because count_mounts() always returns
1 or more for top level direct mounts. Make this clear by using ap->type
in the logical check and rely on the left == 0 check to verify there are
no remaining mounts for indirect mounts since count_mounts() will be
more expensive.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG          |  1 +
 daemon/automount.c | 12 ++++++++----
 daemon/direct.c    | 22 +++++++++++++++++++++-
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index d41527cc1..663ceee8f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -40,6 +40,7 @@
 - remove unnecessary assignment in umount_multi().
 - fix direct mount trigger umount failure case.
 - refactor do_umount_autofs_direct().
+- fix stale direct mount trigger not umounted on expire.
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/automount.c b/daemon/automount.c
index c2c8d8f00..f523a3df9 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -712,10 +712,14 @@ int umount_multi(struct autofs_point *ap, const char *path, int incl)
 
 	left = umount_subtree_mounts(ap, path, is_autofs_fs);
 
-	/* Delete detritus like unwanted mountpoints and symlinks */
-	if (left == 0 &&
-	    ap->state != ST_READMAP &&
-	    !count_mounts(ap, path, ap->dev)) {
+	/* Delete detritus like unwanted mountpoints and symlinks
+	 * for indirect mounts. This can't be done for direct mounts
+	 * here because there's an ioctl file handle open on the
+	 * autofs trigger mount for them so it must be done after
+	 * the expire.
+	 */
+	if (ap->type == LKP_INDIRECT &&
+	    ap->state != ST_READMAP && left == 0) {
 		update_map_cache(ap, path);
 		check_rm_dirs(ap, path, incl);
 	}
diff --git a/daemon/direct.c b/daemon/direct.c
index 6562b183a..90e1a8092 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -1005,10 +1005,30 @@ static void *do_expire_direct(void *arg)
 			       mt.ioctlfd, mt.wait_queue_token, -ENOENT);
 	else {
 		struct mapent *me;
+
 		cache_writelock(mt.mc);
 		me = cache_lookup_distinct(mt.mc, mt.name);
-		if (me)
+		if (me) {
+			/* If the direct mount map entry is no longer
+			 * valid but there is an autofs mount trigger
+			 * for the mount the mount trigger needs to be
+			 * umounted, the map entry deleted and the mount
+			 * point directory removed (if it was created by
+			 * us).
+			 */
 			me->ioctlfd = -1;
+			if (me->mc->map->age > me->age &&
+			    is_mounted(mt.name, MNTS_AUTOFS)) {
+				/* We must detach the mount becuase the
+				 * umount must be completed before
+				 * notifying status to the kernel but
+				 * there's an ioctlfd open on the
+				 * trigger.
+				 */
+				if (!finish_umount(ap, me, -1))
+					cache_delete(me->mc, me->key);
+			}
+		}
 		cache_unlock(mt.mc);
 		ops->send_ready(ap->logopt, mt.ioctlfd, mt.wait_queue_token);
 		ops->close(ap->logopt, mt.ioctlfd);
-- 
2.50.1

[PATCH 5/9] autofs-5.1.9 - remove unnecessary call to set_direct_mount_tree_catatonic()

[Ian Kent]

Function do_umount_autofs_direct() is called in two cases, during a
readmap and when umounting top level direct mounts.

During a readmap, mounts should not be set catatonic so the call to
set_direct_mount_tree_catatonic() is not needed. If it's called for a
top level direct mount the caller, umount_autofs_direct(), calls
set_direct_mount_tree_catatonic() itself already. So remove this
unnecessary call.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 CHANGELOG       | 1 +
 daemon/direct.c | 5 +----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index ebdfe32b7..2d312c9fc 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -36,6 +36,7 @@
 - fix devid update on reload.
 - fix cache writelock must be taken in update_map_cache().
 - fix skip valid map entries on expire cleanup.
+- remove unnecessary call to set_direct_mount_tree_catatonic().
 
 02/11/2023 autofs-5.1.9
 - fix kernel mount status notification.
diff --git a/daemon/direct.c b/daemon/direct.c
index 596201e94..dcd69254d 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -186,11 +186,8 @@ int do_umount_autofs_direct(struct autofs_point *ap, struct mapent *me)
 			warn(ap->logopt, "mount point %s is in use", me->key);
 			if (ap->state == ST_SHUTDOWN_FORCE)
 				goto force_umount;
-			else {
-				if (ap->state != ST_READMAP)
-					set_direct_mount_tree_catatonic(ap, me);
+			else
 				return 0;
-			}
 			break;
 		case ENOTDIR:
 			error(ap->logopt, "mount point is not a directory");
-- 
2.50.1