Re: [RFC] Don't propagate automount

[Ian Kent <raven@themaw.net>]

On Wed, 2019-10-30 at 14:01 +0800, Ian Kent wrote:
> 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@themaw.net>
> > > 
> > > 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@themaw.net>
> > > ---
> > >  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.

And the new variable, shared, initialization is missing too!

> 
> Ian