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.
>
> > --
> >
> > 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 | 11 ++++++++---
> > lib/master_tok.l | 1 +
> > man/auto.master.5.in | 19 ++++++++++---------
> > modules/mount_bind.c | 40 ++++++++++++++++++++++--------------
> > ----
> > 5 files changed, 46 insertions(+), 34 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..e9589a5a 100644
> > --- a/lib/master_parse.y
> > +++ b/lib/master_parse.y
> > @@ -59,6 +59,7 @@ static long timeout;
> > static long negative_timeout;
> > static unsigned symlnk;
> > static unsigned strictexpire;
> > +static unsigned shared;
> > static unsigned slave;
> > static unsigned private;
> > static unsigned nobind;
> > @@ -106,7 +107,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 +209,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,6 +624,7 @@ daemon_option: OPT_TIMEOUT NUMBER { timeout =
> > $2; }
> > | OPT_NTIMEOUT NUMBER { negative_timeout = $2; }
> > | OPT_SYMLINK { symlnk = 1; }
> > | OPT_STRICTEXPIRE { strictexpire = 1; }
> > + | OPT_SHARED { shared = 1; }
> > | OPT_SLAVE { slave = 1; }
> > | OPT_PRIVATE { private = 1; }
> > | OPT_NOBIND { nobind = 1; }
> > @@ -907,8 +910,10 @@ 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;
> > + /* Default is propagation slave */
> > + entry->ap->flags |= MOUNT_FLAG_SLAVE;
> > + if (shared)
> > + entry->ap->flags |= MOUNT_FLAG_SHARED;
> > if (private)
> > entry->ap->flags |= MOUNT_FLAG_PRIVATE;
>
> If the user mention shared or private flag, you will end up
> enabling both MOUNT_FLAG_SLAVE and MOUNT_FLAG_SHARED.
> It would be better to put it in a if..else if..else sequence.
> These are options are mutually exclusive.
Thanks for noticing this obvious blunder.
I'll fix that up and send an update.
Ian
