Re: [PATCH] autofs-5.1.8 - add "unshared" option to disable mount propagation.

Ian Kent <raven@xxxxxxxxxx> · Wed, 22 Mar 2023 09:11:35 +0800

On 20/3/23 11:26, NeilBrown wrote:
[[This is a different approach to meeting my customer's need.
   Thoughts? - thanks.]]

Sometimes it is useful for an autofs mount to be "private" for mount
propagation.  For example, this allows submounts to be moved off with
"mount --move" - such movement is not permitted for shared mounts.

Changing the autofs mount point to "private" with "mount --make-private"
after the automount daemon has started can be problematic.  If the
mount point is already shared there will be another copy that will never
get acted on by automount and so tools accessing it can hang.

So to safely achieve non-shared auto-mountpoints we need to create a
transparent bind mount at the mount point, mark it as private, then
mount the autofs mount there.

This patch does this when the "unshared" option is given in the
auto.master file.

Ideally we would check if the target mountpoint is shared and skip the
extra bind mount in that case.  However checking the propagation status
requires reading /proc/self/mountinfo which is more work than seems
justified in this case.

Signed-off-by: NeilBrown <neilb@xxxxxxx>
---
  daemon/automount.c    | 20 ++++++++++++++++----
  daemon/direct.c       | 26 ++++++++++++++++++++++++++
  daemon/indirect.c     | 18 ++++++++++++++++++
  daemon/master_parse.y |  7 +++++++
  daemon/master_tok.l   |  1 +
  include/automount.h   |  6 ++++++
  man/auto.master.5.in  |  9 +++++++++
  7 files changed, 83 insertions(+), 4 deletions(-)

diff --git a/daemon/automount.c b/daemon/automount.c
index f550bc8fb222..dbc8976550eb 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -32,6 +32,7 @@
  #include <sys/stat.h>
  #include <sys/time.h>
  #include <sys/resource.h>
+#include <sys/mount.h>
  #include <sys/poll.h>
  #include <dirent.h>
  #include <sys/vfs.h>
@@ -1739,7 +1740,7 @@ static void handle_mounts_cleanup(void *arg)
  {
  	struct autofs_point *ap;
  	char buf[MAX_ERR_BUF];
-	unsigned int clean = 0, submount, logopt;
+	unsigned int clean = 0, clean_bind = 0, submount, logopt;
  	unsigned int pending = 0;
  
  	ap = (struct autofs_point *) arg;
@@ -1747,9 +1748,12 @@ static void handle_mounts_cleanup(void *arg)
  	logopt = ap->logopt;
  	submount = ap->submount;
  
-	if (!submount && strcmp(ap->path, "/-") &&
-	    ap->flags & MOUNT_FLAG_DIR_CREATED)
-		clean = 1;
+	if (!submount && strcmp(ap->path, "/-")) {
+		if (ap->flags & MOUNT_FLAG_DIR_CREATED)
+			clean = 1;
+		if (ap->flags & MOUNT_FLAG_DID_BIND)
+			clean_bind = 1;
+	}

I think I have a bug here and you've been caught by it.


Not considering the directory created flag, that if clause above

says "if it's an indirect mount and it's not a submount" but below

there's an extra mount for both indirect and direct mounts. Looks

like it leaks direct mounts. I think I might require direct mount

point directories to already exist, so maybe it's not a bug before

the change here, I'd need to check.


Now direct mounts do need to be distinct (no nesting is allowed)

so that should be fine but we frequently see direct mount maps

with several hundred or less frequently several thousand map

entries and occasionally many more. So making each one consume 2

mounts is probably not such a good idea. Although it does require

a mount option so maybe it's ok.


Ian

  
  	if (submount) {
  		struct mnt_list *mnt;
@@ -1787,6 +1791,14 @@ static void handle_mounts_cleanup(void *arg)
  		master_free_mapent(ap->entry);
  	}
  
+	if (clean_bind) {
+		if (umount2(ap->path, MNT_DETACH)) {
+			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
+			warn(logopt, "failed to remove internal bind mount %s: %s",
+			     ap->path, estr);
+		}
+	}
+
  	if (clean) {
  		if (rmdir(ap->path) == -1) {
  			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
diff --git a/daemon/direct.c b/daemon/direct.c
index 316ffd781762..db15689def9d 100644
--- a/daemon/direct.c
+++ b/daemon/direct.c
@@ -205,6 +205,14 @@ force_umount:
  	} else
  		info(ap->logopt, "umounted direct mount %s", me->key);
  
+	if (!rv && (me->flags & MOUNT_FLAG_DID_BIND)) {
+		if (umount2(me->key, MNT_DETACH) == -1) {
+			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
+			warn(ap->logopt, "failed to unmount internal bind mount %s: %s",
+			     me->key, estr);
+		}
+	}
+
  	if (!rv && me->flags & MOUNT_FLAG_DIR_CREATED) {
  		if  (rmdir(me->key) == -1) {
  			char *estr = strerror_r(errno, buf, MAX_ERR_BUF);
@@ -400,6 +408,24 @@ int do_mount_autofs_direct(struct autofs_point *ap,
  
  	map_name = me->mc->map->argv[0];
  
+	me->flags &= ~MOUNT_FLAG_DID_BIND;
+	if (ap->flags & MOUNT_FLAG_UNSHARED) {
+		/* Ideally we would check if the mount is shared, but
+		 * that requires reading mountinfo which isn't easy.
+		 */
+		if (mount(me->key, me->key, "none", MS_BIND, NULL) != 0) {
+			error(ap->logopt,
+			      "failed to create internal bind mount for 'unshared': %s",
+			      me->key);
+		} else {
+			if (mount(NULL, me->key, NULL, MS_PRIVATE, NULL) == 0)
+				me->flags |= MOUNT_FLAG_DID_BIND;
+			else
+				error(ap->logopt,
+				      "failed to make %s private", me->key);
+		}
+	}
+
  	ret = mount(map_name, me->key, "autofs", MS_MGC_VAL, mp->options);
  	if (ret) {
  		crit(ap->logopt, "failed to mount autofs path %s", me->key);
diff --git a/daemon/indirect.c b/daemon/indirect.c
index 23ef9f41398d..70c601814f6d 100644
--- a/daemon/indirect.c
+++ b/daemon/indirect.c
@@ -108,6 +108,24 @@ static int do_mount_autofs_indirect(struct autofs_point *ap, const char *root)
  	if (!type || strcmp(ap->entry->maps->type, "hosts"))
  		map_name = ap->entry->maps->argv[0];
  
+	ap->flags &= ~MOUNT_FLAG_DID_BIND;
+	if (ap->flags & MOUNT_FLAG_UNSHARED) {
+		/* Ideally we would check if the mount is shared, but
+		 * that requires reading mountinfo which isn't easy.
+		 */
+		if (mount(root, root, "none", MS_BIND, NULL) != 0) {
+			error(ap->logopt,
+			      "failed to create internal bind mount for 'unshared': %s",
+			      root);
+		} else {
+			if (mount(NULL, root, NULL, MS_PRIVATE, NULL) == 0)
+				ap->flags |= MOUNT_FLAG_DID_BIND;
+			else
+				error(ap->logopt,
+				      "failed to make %s private", root);
+		}
+	}
+
  	ret = mount(map_name, root, "autofs", MS_MGC_VAL, options);
  	if (ret) {
  		crit(ap->logopt,
diff --git a/daemon/master_parse.y b/daemon/master_parse.y
index 469cfe9765ac..9e4e9d0e02ff 100644
--- a/daemon/master_parse.y
+++ b/daemon/master_parse.y
@@ -62,6 +62,7 @@ static long positive_timeout;
  static unsigned symlnk;
  static unsigned strictexpire;
  static unsigned nobind;
+static unsigned unshared;
  static unsigned ghost;
  extern unsigned global_selection_options;
  static unsigned random_selection;
@@ -116,6 +117,7 @@ static int master_fprintf(FILE *, char *, ...);
  %token OPT_TIMEOUT OPT_NTIMEOUT OPT_PTIMEOUT OPT_NOBIND OPT_NOGHOST OPT_GHOST OPT_VERBOSE
  %token OPT_DEBUG OPT_RANDOM OPT_USE_WEIGHT OPT_SYMLINK OPT_MODE
  %token OPT_STRICTEXPIRE OPT_SHARED OPT_SLAVE OPT_PRIVATE
+%token OPT_UNSHARED
  %token COLON COMMA NL DDASH
  %type <strtype> map
  %type <strtype> options
@@ -223,6 +225,7 @@ line:
  	| PATH OPT_SLAVE { master_notify($1); YYABORT; }
  	| PATH OPT_PRIVATE { master_notify($1); YYABORT; }
  	| PATH OPT_NOBIND { master_notify($1); YYABORT; }
+	| PATH OPT_UNSHARED { master_notify($1); YYABORT; }
  	| PATH OPT_GHOST { master_notify($1); YYABORT; }
  	| PATH OPT_NOGHOST { master_notify($1); YYABORT; }
  	| PATH OPT_VERBOSE { master_notify($1); YYABORT; }
@@ -642,6 +645,7 @@ daemon_option: OPT_TIMEOUT NUMBER { timeout = $2; }
  	| OPT_SHARED	{ propagation = PROPAGATION_SHARED; }
  	| OPT_SLAVE	{ propagation = PROPAGATION_SLAVE; }
  	| OPT_PRIVATE	{ propagation = PROPAGATION_PRIVATE; }
+	| OPT_UNSHARED  { unshared = 1; }
  	| OPT_NOBIND	{ nobind = 1; }
  	| OPT_NOGHOST	{ ghost = 0; }
  	| OPT_GHOST	{ ghost = 1; }
@@ -716,6 +720,7 @@ static void local_init_vars(void)
  	symlnk = 0;
  	strictexpire = 0;
  	propagation = PROPAGATION_SLAVE;
+	unshared = 0;
  	nobind = 0;
  	ghost = defaults_get_browse_mode();
  	random_selection = global_selection_options & MOUNT_FLAG_RANDOM_SELECT;
@@ -918,6 +923,8 @@ int master_parse_entry(const char *buffer, unsigned int default_timeout, unsigne
  	entry->ap->flags &= ~(PROPAGATION_MASK);
  	entry->ap->flags |= propagation;
  
+	if (unshared)
+		entry->ap->flags |= MOUNT_FLAG_UNSHARED;
  	if (random_selection)
  		entry->ap->flags |= MOUNT_FLAG_RANDOM_SELECT;
  	if (use_weight)
diff --git a/daemon/master_tok.l b/daemon/master_tok.l
index e2d15bce8b4d..28b34647ecc6 100644
--- a/daemon/master_tok.l
+++ b/daemon/master_tok.l
@@ -437,6 +437,7 @@ MODE		(--mode{OPTWS}|--mode{OPTWS}={OPTWS})
  	-?shared		{ return(OPT_SHARED); }
  	-?slave			{ return(OPT_SLAVE); }
  	-?private		{ return(OPT_PRIVATE); }
+	-?unshared		{ return(OPT_UNSHARED); }
  	-?strictexpire		{ return(OPT_STRICTEXPIRE); }
  	-g|--ghost|-?browse	{ return(OPT_GHOST); }
  	-v|--verbose		{ return(OPT_VERBOSE); }
diff --git a/include/automount.h b/include/automount.h
index 9387ad1b1ce2..32372c5cabce 100644
--- a/include/automount.h
+++ b/include/automount.h
@@ -548,6 +548,12 @@ struct kernel_mod_version {
  /* Indicator for applications to ignore the mount entry */
  #define MOUNT_FLAG_IGNORE		0x1000
  
+/* The autofs mount must not be shared */
+#define MOUNT_FLAG_UNSHARED		0x2000
+
+/* The bind mount was created first to change propagation */
+#define MOUNT_FLAG_DID_BIND		0x4000
+
  struct autofs_point {
  	pthread_t thid;
  	char *path;			/* Mount point name */
diff --git a/man/auto.master.5.in b/man/auto.master.5.in
index 167170150292..db907b6bc878 100644
--- a/man/auto.master.5.in
+++ b/man/auto.master.5.in
@@ -221,6 +221,15 @@ since the automount target is itself an (unwanted) automount trigger.
  This option is an autofs pseudo mount option that can be used in the
  master map only.
  .TP
+.I unshared
+This option ensures that the auto-mountpoint created is not shared for
+the purposes of mount propagation.  Marking the mountpoint "private"
+after it is mounted is not safe for auto-mountpoints as it can leave
+orphans auto-mountpoints which will causing any tool accessing them to
+hang.  So automount must ensure the auto-mountpoints are not shared when
+first mounted.  It does this by creating an internal bind mount at the
+target mountpoint which has mount propagation disabled.
+.TP
  .I "\-r, \-\-random-multimount-selection"
  Enables the use of random selection when choosing a host from a
  list of replicated servers. This option is applied to this mount






