Re: [RFC] Don't propagate automount

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

 



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.

It may not affect your current implementation, it will
show up as a bug if there are further changes in the
mount() sequence.

-- 
Goldwyn

>  	if (negative_timeout)
> 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..58132d69 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 the 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;
> 



[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