autofs-5.1.7 - fix inconsistent locking in parse_mount() From: Ian Kent 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 --- 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);