Re: [RFC] Don't propagate automount

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux