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);