From: Ian Kent <raven@themaw.net>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: viro@zeniv.linux.org.uk, autofs@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [RFC] Don't propagate automount
Date: Wed, 30 Oct 2019 14:01:21 +0800 [thread overview]
Message-ID: <a7e2d30f30f435216e9533d79928e6cf2e953256.camel@themaw.net> (raw)
In-Reply-To: <20191029160017.33chq3w2alyzscfa@fiona>
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.
Ian
next prev parent reply other threads:[~2019-10-30 6:01 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-26 19:52 [RFC] Don't propagate automount Goldwyn Rodrigues
2019-09-27 1:35 ` Ian Kent
2019-09-27 7:09 ` Ian Kent
2019-09-27 7:26 ` Ian Kent
2019-09-27 7:41 ` Ian Kent
2019-09-27 10:51 ` Ian Kent
2019-09-27 16:16 ` Goldwyn Rodrigues
2019-09-28 1:47 ` Ian Kent
2019-10-01 19:09 ` Goldwyn Rodrigues
2019-10-02 2:14 ` Ian Kent
2019-10-28 16:28 ` Goldwyn Rodrigues
2019-10-28 23:57 ` Ian Kent
2019-10-29 6:39 ` Ian Kent
2019-10-29 6:40 ` Ian Kent
2019-10-29 16:00 ` Goldwyn Rodrigues
2019-10-30 6:01 ` Ian Kent [this message]
2019-10-30 6:05 ` Ian Kent
2019-10-30 12:05 ` Ian Kent
2019-10-30 19:28 ` Goldwyn Rodrigues
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a7e2d30f30f435216e9533d79928e6cf2e953256.camel@themaw.net \
--to=raven@themaw.net \
--cc=autofs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=rgoldwyn@suse.de \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).