On Tue, 2019-10-29 at 11:00 -0500, Goldwyn Rodrigues wrote:
> On 14:39 29/10, Ian Kent wrote:
> > On Tue, 2019-10-29 at 07:57 +0800, Ian Kent wrote:
> > > On Mon, 2019-10-28 at 11:28 -0500, Goldwyn Rodrigues wrote:
> > > > Hi Ian,
> > > >
> > > > On 10:14 02/10, Ian Kent wrote:
> > > > > On Tue, 2019-10-01 at 14:09 -0500, Goldwyn Rodrigues wrote:
> > > > <snip>
> > > >
> > > > > Anyway, it does sound like it's worth putting time into
> > > > > your suggestion of a kernel change.
> > > > >
> > > > > Unfortunately I think it's going to take a while to work
> > > > > out what's actually going on with the propagation and I'm
> > > > > in the middle of some other pressing work right now.
> > > >
> > > > Have you have made any progress on this issue?
> > >
> > > Sorry I haven't.
> > > I still want to try and understand what's going on there.
> > >
> > > > As I mentioned, I am fine with a userspace solution of
> > > > defaulting
> > > > to slave mounts all of the time instead of this kernel patch.
> > >
> > > Oh, I thought you weren't keen on that recommendation.
> > >
> > > That shouldn't take long to do so I should be able to get that
> > > done
> > > and post a patch pretty soon.
> > >
> > > I'll get back to looking at the mount propagation code when I get
> > > a
> > > chance. Unfortunately I haven't been very successful when making
> > > changes to that area of code in the past ...
> >
> > After working on this patch I'm even more inclined to let the
> > kernel
> > do it's propagation thing and set the autofs mounts, either
> > silently
> > by default or explicitly by map entry option.
> >
> > Because it's the propagation setting of the parent mount that
> > controls
> > the propagation of its children there shouldn't be any chance of a
> > race so this should be reliable.
> >
> > Anyway, here is a patch, compile tested only, and without the
> > changelog
> > hunk I normally add to save you possible conflicts. But unless
> > there
> > are any problems found this is what I will eventually commit to the
> > repo.
> >
> > If there are any changes your not aware of I'll let you know.
> >
> > Clearly this depends on the other two related patches for this
> > issue.
>
> This works good for us. Thanks.
> However, I have some review comments for the patch.
I think this one should do the trick.
autofs-5.1.6 - make bind mounts propagation slave by default
From: Ian Kent <raven@xxxxxxxxxx>
Make setting mount propagation on bind mounts mandatory with a default
of propagation slave.
When using multi-mounts that have bind mounts the bind mount will have
the same properties as its parent which is commonly propagation shared.
And if the mount target is also propagation shared this can lead to a
deadlock when attempting to access the offset mounts. When this happens
an unwanted offset mount is propagated back to the target file system
resulting in a deadlock since the automount target is itself an
(unwanted) automount trigger.
This problem has been present much longer than I originally thought,
perhaps since mount propagation was introduced into the kernel, so
explicitly setting bind mount propagation is the sensible thing to do.
Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
---
include/automount.h | 9 +++++----
lib/master_parse.y | 24 +++++++++++++-----------
lib/master_tok.l | 1 +
man/auto.master.5.in | 19 ++++++++++---------
modules/mount_bind.c | 40 ++++++++++++++++++++++------------------
5 files changed, 51 insertions(+), 42 deletions(-)
diff --git a/include/automount.h b/include/automount.h
index 4fd0ba96..fe9c7fee 100644
--- a/include/automount.h
+++ b/include/automount.h
@@ -551,14 +551,15 @@ struct kernel_mod_version {
#define MOUNT_FLAG_AMD_CACHE_ALL 0x0080
/* Set mount propagation for bind mounts */
-#define MOUNT_FLAG_SLAVE 0x0100
-#define MOUNT_FLAG_PRIVATE 0x0200
+#define MOUNT_FLAG_SHARED 0x0100
+#define MOUNT_FLAG_SLAVE 0x0200
+#define MOUNT_FLAG_PRIVATE 0x0400
/* Use strict expire semantics if requested and kernel supports it */
-#define MOUNT_FLAG_STRICTEXPIRE 0x0400
+#define MOUNT_FLAG_STRICTEXPIRE 0x0800
/* Indicator for applications to ignore the mount entry */
-#define MOUNT_FLAG_IGNORE 0x0800
+#define MOUNT_FLAG_IGNORE 0x1000
struct autofs_point {
pthread_t thid;
diff --git a/lib/master_parse.y b/lib/master_parse.y
index f817f739..3f36b0aa 100644
--- a/lib/master_parse.y
+++ b/lib/master_parse.y
@@ -59,8 +59,6 @@ static long timeout;
static long negative_timeout;
static unsigned symlnk;
static unsigned strictexpire;
-static unsigned slave;
-static unsigned private;
static unsigned nobind;
static unsigned ghost;
extern unsigned global_selection_options;
@@ -72,6 +70,11 @@ static int tmp_argc;
static char **local_argv;
static int local_argc;
+#define PROPAGATION_SHARED MOUNT_FLAG_SHARED
+#define PROPAGATION_SLAVE MOUNT_FLAG_SLAVE
+#define PROPAGATION_PRIVATE MOUNT_FLAG_PRIVATE
+static unsigned int propagation;
+
static char errstr[MAX_ERR_LEN];
static unsigned int verbose;
@@ -106,7 +109,7 @@ static int master_fprintf(FILE *, char *, ...);
%token MAP
%token OPT_TIMEOUT OPT_NTIMEOUT OPT_NOBIND OPT_NOGHOST OPT_GHOST OPT_VERBOSE
%token OPT_DEBUG OPT_RANDOM OPT_USE_WEIGHT OPT_SYMLINK OPT_MODE
-%token OPT_STRICTEXPIRE OPT_SLAVE OPT_PRIVATE
+%token OPT_STRICTEXPIRE OPT_SHARED OPT_SLAVE OPT_PRIVATE
%token COLON COMMA NL DDASH
%type <strtype> map
%type <strtype> options
@@ -208,6 +211,7 @@ line:
| PATH OPT_TIMEOUT { master_notify($1); YYABORT; }
| PATH OPT_SYMLINK { master_notify($1); YYABORT; }
| PATH OPT_STRICTEXPIRE { master_notify($1); YYABORT; }
+ | PATH OPT_SHARED { master_notify($1); YYABORT; }
| PATH OPT_SLAVE { master_notify($1); YYABORT; }
| PATH OPT_PRIVATE { master_notify($1); YYABORT; }
| PATH OPT_NOBIND { master_notify($1); YYABORT; }
@@ -622,8 +626,9 @@ daemon_option: OPT_TIMEOUT NUMBER { timeout = $2; }
| OPT_NTIMEOUT NUMBER { negative_timeout = $2; }
| OPT_SYMLINK { symlnk = 1; }
| OPT_STRICTEXPIRE { strictexpire = 1; }
- | OPT_SLAVE { slave = 1; }
- | OPT_PRIVATE { private = 1; }
+ | OPT_SHARED { propagation = PROPAGATION_SHARED; }
+ | OPT_SLAVE { propagation = PROPAGATION_SLAVE; }
+ | OPT_PRIVATE { propagation = PROPAGATION_PRIVATE; }
| OPT_NOBIND { nobind = 1; }
| OPT_NOGHOST { ghost = 0; }
| OPT_GHOST { ghost = 1; }
@@ -697,8 +702,7 @@ static void local_init_vars(void)
negative_timeout = 0;
symlnk = 0;
strictexpire = 0;
- slave = 0;
- private = 0;
+ propagation = PROPAGATION_SLAVE;
nobind = 0;
ghost = defaults_get_browse_mode();
random_selection = global_selection_options & MOUNT_FLAG_RANDOM_SELECT;
@@ -899,6 +903,8 @@ int master_parse_entry(const char *buffer, unsigned int default_timeout, unsigne
return 0;
}
}
+ entry->ap->flags = propagation;
+
if (random_selection)
entry->ap->flags |= MOUNT_FLAG_RANDOM_SELECT;
if (use_weight)
@@ -907,10 +913,6 @@ int master_parse_entry(const char *buffer, unsigned int default_timeout, unsigne
entry->ap->flags |= MOUNT_FLAG_SYMLINK;
if (strictexpire)
entry->ap->flags |= MOUNT_FLAG_STRICTEXPIRE;
- if (slave)
- entry->ap->flags |= MOUNT_FLAG_SLAVE;
- if (private)
- entry->ap->flags |= MOUNT_FLAG_PRIVATE;
if (negative_timeout)
entry->ap->negative_timeout = negative_timeout;
if (mode && mode < LONG_MAX)
diff --git a/lib/master_tok.l b/lib/master_tok.l
index 7486710b..87a6b958 100644
--- a/lib/master_tok.l
+++ b/lib/master_tok.l
@@ -389,6 +389,7 @@ MODE (--mode{OPTWS}|--mode{OPTWS}={OPTWS})
-?symlink { return(OPT_SYMLINK); }
-?nobind { return(OPT_NOBIND); }
-?nobrowse { return(OPT_NOGHOST); }
+ -?shared { return(OPT_SHARED); }
-?slave { return(OPT_SLAVE); }
-?private { return(OPT_PRIVATE); }
-?strictexpire { return(OPT_STRICTEXPIRE); }
diff --git a/man/auto.master.5.in b/man/auto.master.5.in
index 6e510a59..2a0b672a 100644
--- a/man/auto.master.5.in
+++ b/man/auto.master.5.in
@@ -208,17 +208,18 @@ applications scanning the mount tree. Note that this doesn't completely
resolve the problem of expired automounts being immediately re-mounted
due to application accesses triggered by the expire itself.
.TP
-.I slave \fPor\fI private
+.I slave\fP, \fIprivate\fP or \fIshared\fP
This option allows mount propagation of bind mounts to be set to
-either \fIslave\fP or \fIprivate\fP. This option may be needed when using
-multi-mounts that have bind mounts that bind to a file system that is
-propagation shared. This is because the bind mount will have the same
-properties as its target which causes problems for offset mounts. When
-this happens an unwanted offset mount is propagated back to the target
-file system resulting in a deadlock when attempting to access the offset.
+\fIslave\fP, \fIprivate\fP or \fIshared\fP. This option defaults to
+\fIslave\fP if no option is given. When using multi-mounts that have
+bind mounts the bind mount will have the same properties as its parent
+which is commonly propagation \fIshared\fP. And if the mount target is
+also propagation \fIshared\fP this can lead to a deadlock when attempting
+to access the offset mounts. When this happens an unwanted offset mount
+is propagated back to the target file system resulting in a deadlock
+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. By default, bind mounts will inherit the mount propagation
-of the target file system.
+master map only.
.TP
.I "\-r, \-\-random-multimount-selection"
Enables the use of random selection when choosing a host from a
diff --git a/modules/mount_bind.c b/modules/mount_bind.c
index 9cba0d7a..5253501c 100644
--- a/modules/mount_bind.c
+++ b/modules/mount_bind.c
@@ -153,6 +153,7 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int
if (!symlnk && bind_works) {
int status, existed = 1;
+ int flags;
debug(ap->logopt, MODPREFIX "calling mkdir_path %s", fullpath);
@@ -190,24 +191,27 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int
what, fstype, fullpath);
}
- if (ap->flags & (MOUNT_FLAG_SLAVE | MOUNT_FLAG_PRIVATE)) {
- int flags = MS_SLAVE;
-
- if (ap->flags & MOUNT_FLAG_PRIVATE)
- flags = MS_PRIVATE;
-
- /* The bind mount has succeeded but if the target
- * mount is propagation shared propagation of child
- * mounts (autofs offset mounts for example) back to
- * the target of the bind mount must be avoided or
- * autofs trigger mounts will deadlock.
- */
- err = mount(NULL, fullpath, NULL, flags, NULL);
- if (err) {
- warn(ap->logopt,
- "failed to set propagation for %s",
- fullpath, root);
- }
+ /* The bind mount has succeeded, now set the mount propagation.
+ *
+ * The default is propagation shared, change it if the master
+ * map entry has a different option specified.
+ */
+ flags = MS_SLAVE;
+ if (ap->flags & MOUNT_FLAG_PRIVATE)
+ flags = MS_PRIVATE;
+ else if (ap->flags & MOUNT_FLAG_SHARED)
+ flags = MS_SHARED;
+
+ /* Note: If the parent mount is propagation shared propagation
+ * of child mounts (autofs offset mounts for example) back to
+ * the target of the bind mount can happen in some cases and
+ * must be avoided or autofs trigger mounts will deadlock.
+ */
+ err = mount(NULL, fullpath, NULL, flags, NULL);
+ if (err) {
+ warn(ap->logopt,
+ "failed to set propagation for %s",
+ fullpath, root);
}
return 0;
