autofs-5.1.7 - fix inconsistent locking in parse_mount()

From: Ian Kent <raven@themaw.net>

Some map entry cache locking inconsistencies have crept in.

In parse_mount() of the sun format parser the cache read lock is too

heavily used and has too broad a scope. This has lead to some operations

that should hold the write lock being called with only the read lock.

Signed-off-by: Ian Kent <raven@themaw.net>

---

 CHANGELOG           |    1 +

 lib/mounts.c        |    9 ++++++++-

 modules/parse_sun.c |   53 ++++++++++++++++++++++++++++++++-------------------

 3 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG

index c60a9ed3..d25b19c8 100644

--- a/CHANGELOG

+++ b/CHANGELOG

@@ -18,6 +18,7 @@

 - fix inconsistent locking in umount_subtree_mounts().

 - fix return from umount_subtree_mounts() on offset list delete.

 - pass mapent_cache to update_offset_entry().

+- fix inconsistent locking in parse_mount().

 25/01/2021 autofs-5.1.7

 - make bind mounts propagation slave by default.

diff --git a/lib/mounts.c b/lib/mounts.c

index 5ebfe5fd..0fcd4087 100644

--- a/lib/mounts.c

+++ b/lib/mounts.c

@@ -2491,6 +2491,12 @@ static int do_mount_autofs_offset(struct autofs_point *ap,

 		else {

 			debug(ap->logopt, "ignoring \"nohide\" trigger %s",

 			      oe->key);

+			/*

+			 * Ok, so we shouldn't modify the mapent but

+			 * mount requests are blocked at a point above

+			 * this and expire only uses the mapent key or

+			 * holds the cache write lock.

+			 */

 			free(oe->mapent);

 			oe->mapent = NULL;

 		}

@@ -2634,7 +2640,8 @@ static int do_umount_offset(struct autofs_point *ap, struct mapent *oe, const ch

 			/*

 			 * Ok, so we shouldn't modify the mapent but

 			 * mount requests are blocked at a point above

-			 * this and expire only uses the mapent key.

+			 * this and expire only uses the mapent key or

+			 * holds the cache write lock.

 			 */

 			if (oe->mapent) {

 				free(oe->mapent);

diff --git a/modules/parse_sun.c b/modules/parse_sun.c

index 95251bee..a6630a76 100644

--- a/modules/parse_sun.c

+++ b/modules/parse_sun.c

@@ -851,10 +851,12 @@ update_offset_entry(struct autofs_point *ap,

 			strcpy(m_mapent, loc);

 	}

+	cache_writelock(mc);

 	ret = cache_update_offset(mc, name, m_key, m_mapent, age);

 	if (!cache_set_offset_parent(mc, m_key))

 		error(ap->logopt, "failed to set offset parent");

+	cache_unlock(mc);

 	if (ret == CHE_DUPLICATE) {

 		warn(ap->logopt, MODPREFIX

@@ -1128,14 +1130,22 @@ static void cleanup_multi_triggers(struct autofs_point *ap,

 	return;

 }

-static int mount_subtree(struct autofs_point *ap, struct mapent *me,

+static int mount_subtree(struct autofs_point *ap, struct mapent_cache *mc,

 			 const char *name, char *loc, char *options, void *ctxt)

 {

+	struct mapent *me;

 	struct mapent *ro;

 	char *mm_root, *mm_base, *mm_key;

 	unsigned int mm_root_len;

 	int start, ret = 0, rv;

+	cache_readlock(mc);

+	me = cache_lookup_distinct(mc, name);

+	if (!me) {

+		cache_unlock(mc);

+		return 0;

+	}

+

 	rv = 0;

 	mm_key = me->multi->key;

@@ -1180,9 +1190,12 @@ static int mount_subtree(struct autofs_point *ap, struct mapent *me,

 			rv = parse_mapent(ro->mapent,

 				options, &myoptions, &ro_loc, ap->logopt);

 			if (!rv) {

+				cache_unlock(mc);

 				warn(ap->logopt,

 				      MODPREFIX "failed to parse root offset");

-				cache_delete_offset_list(me->mc, name);

+				cache_writelock(mc);

+				cache_delete_offset_list(mc, name);

+				cache_unlock(mc);

 				return 1;

 			}

 			ro_len = 0;

@@ -1199,9 +1212,10 @@ static int mount_subtree(struct autofs_point *ap, struct mapent *me,

 		if ((ro && rv == 0) || rv <= 0) {

 			ret = mount_multi_triggers(ap, me, mm_root, start, mm_base);

 			if (ret == -1) {

+				cleanup_multi_triggers(ap, me, mm_root, start, mm_base);

+				cache_unlock(mc);

 				error(ap->logopt, MODPREFIX

 					 "failed to mount offset triggers");

-				cleanup_multi_triggers(ap, me, mm_root, start, mm_base);

 				return 1;

 			}

 		}

@@ -1217,9 +1231,10 @@ static int mount_subtree(struct autofs_point *ap, struct mapent *me,

 		if (rv == 0) {

 			ret = mount_multi_triggers(ap, me->multi, name, start, mm_base);

 			if (ret == -1) {

+				cleanup_multi_triggers(ap, me, name, start, mm_base);

+				cache_unlock(mc);

 				error(ap->logopt, MODPREFIX

 					 "failed to mount offset triggers");

-				cleanup_multi_triggers(ap, me, name, start, mm_base);

 				return 1;

 			}

 		} else if (rv < 0) {

@@ -1227,8 +1242,11 @@ static int mount_subtree(struct autofs_point *ap, struct mapent *me,

 			unsigned int mm_root_base_len = mm_root_len + strlen(mm_base) + 1;

 			if (mm_root_base_len > PATH_MAX) {

+				cache_unlock(mc);

 				warn(ap->logopt, MODPREFIX "path too long");

-				cache_delete_offset_list(me->mc, name);

+				cache_writelock(mc);

+				cache_delete_offset_list(mc, name);

+				cache_unlock(mc);

 				return 1;

 			}

@@ -1237,13 +1255,15 @@ static int mount_subtree(struct autofs_point *ap, struct mapent *me,

 			ret = mount_multi_triggers(ap, me->multi, mm_root_base, start, mm_base);

 			if (ret == -1) {

+				cleanup_multi_triggers(ap, me, mm_root, start, mm_base);

+				cache_unlock(mc);

 				error(ap->logopt, MODPREFIX

 					 "failed to mount offset triggers");

-				cleanup_multi_triggers(ap, me, mm_root, start, mm_base);

 				return 1;

 			}

 		}

 	}

+	cache_unlock(mc);

 	/* Mount for base of tree failed */

 	if (rv > 0)

@@ -1484,7 +1504,6 @@ dont_expand:

 			return 1;

 		}

-		cache_multi_writelock(me);

 		/* So we know we're the multi-mount root */

 		if (!me->multi)

 			me->multi = me;

@@ -1509,14 +1528,13 @@ dont_expand:

 			if (source->flags & MAP_FLAG_FORMAT_AMD) {

 				free(options);

 				free(pmapent);

-				cache_multi_unlock(me);

 				cache_unlock(mc);

 				pthread_setcancelstate(cur_state, NULL);

 				return 0;

 			}

 		}

-

 		age = me->age;

+		cache_unlock(mc);

 		/* It's a multi-mount; deal with it */

 		do {

@@ -1537,8 +1555,8 @@ dont_expand:

 			if (!path) {

 				warn(ap->logopt, MODPREFIX "null path or out of memory");

+				cache_writelock(mc);

 				cache_delete_offset_list(mc, name);

-				cache_multi_unlock(me);

 				cache_unlock(mc);

 				free(options);

 				free(pmapent);

@@ -1554,8 +1572,8 @@ dont_expand:

 			l = parse_mapent(p, options, &myoptions, &loc, ap->logopt);

 			if (!l) {

+				cache_writelock(mc);

 				cache_delete_offset_list(mc, name);

-				cache_multi_unlock(me);

 				cache_unlock(mc);

 				free(path);

 				free(options);

@@ -1573,8 +1591,8 @@ dont_expand:

 			if (status != CHE_OK) {

 				warn(ap->logopt, MODPREFIX "error adding multi-mount");

+				cache_writelock(mc);

 				cache_delete_offset_list(mc, name);

-				cache_multi_unlock(me);

 				cache_unlock(mc);

 				free(path);

 				free(options);

@@ -1592,10 +1610,7 @@ dont_expand:

 			free(myoptions);

 		} while (*p == '/' || (*p == '"' && *(p + 1) == '/'));

-		rv = mount_subtree(ap, me, name, NULL, options, ctxt);

-

-		cache_multi_unlock(me);

-		cache_unlock(mc);

+		rv = mount_subtree(ap, mc, name, NULL, options, ctxt);

 		free(options);

 		free(pmapent);

@@ -1616,6 +1631,7 @@ dont_expand:

 		cache_readlock(mc);

 		if (*name == '/' &&

 		   (me = cache_lookup_distinct(mc, name)) && me->multi) {

+			cache_unlock(mc);

 			loc = strdup(p);

 			if (!loc) {

 				free(options);

@@ -1624,10 +1640,7 @@ dont_expand:

 				warn(ap->logopt, MODPREFIX "out of memory");

 				return 1;

 			}

-			cache_multi_writelock(me);

-			rv = mount_subtree(ap, me, name, loc, options, ctxt);

-			cache_multi_unlock(me);

-			cache_unlock(mc);

+			rv = mount_subtree(ap, mc, name, loc, options, ctxt);

 			free(loc);

 			free(options);

 			free(pmapent);