Re: [PATCH v5] Add a "nosymfollow" mount option.

From: Aleksa Sarai <cyphar@cyphar.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Ross Zwisler <zwisler@chromium.org>,
	Raul Rangel <rrangel@google.com>,
	David Howells <dhowells@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Mattias Nissler <mnissler@chromium.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org,
	Benjamin Gordon <bmgordon@google.com>,
	Micah Morton <mortonm@google.com>,
	Dmitry Torokhov <dtor@google.com>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH v5] Add a "nosymfollow" mount option.
Date: Wed, 5 Feb 2020 14:45:00 +1100	[thread overview]
Message-ID: <20200205034500.x3omkziqwu3g5gpx@yavin> (raw)
In-Reply-To: <20200205032110.GR8731@bombadil.infradead.org>

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

On 2020-02-04, Matthew Wilcox <willy@infradead.org> wrote:
> On Tue, Feb 04, 2020 at 04:49:48PM -0700, Ross Zwisler wrote:
> > On Tue, Feb 4, 2020 at 3:11 PM Ross Zwisler <zwisler@chromium.org> wrote:
> > > On Tue, Feb 4, 2020 at 2:53 PM Raul Rangel <rrangel@google.com> wrote:
> > > > > --- a/include/uapi/linux/mount.h
> > > > > +++ b/include/uapi/linux/mount.h
> > > > > @@ -34,6 +34,7 @@
> > > > >  #define MS_I_VERSION   (1<<23) /* Update inode I_version field */
> > > > >  #define MS_STRICTATIME (1<<24) /* Always perform atime updates */
> > > > >  #define MS_LAZYTIME    (1<<25) /* Update the on-disk [acm]times lazily */
> > > > > +#define MS_NOSYMFOLLOW (1<<26) /* Do not follow symlinks */
> > > > Doesn't this conflict with MS_SUBMOUNT below?
> > > > >
> > > > >  /* These sb flags are internal to the kernel */
> > > > >  #define MS_SUBMOUNT     (1<<26)
> > >
> > > Yep.  Thanks for the catch, v6 on it's way.
> > 
> > It actually looks like most of the flags which are internal to the
> > kernel are actually unused (MS_SUBMOUNT, MS_NOREMOTELOCK, MS_NOSEC,
> > MS_BORN and MS_ACTIVE).  Several are unused completely, and the rest
> > are just part of the AA_MS_IGNORE_MASK which masks them off in the
> > apparmor LSM, but I'm pretty sure they couldn't have been set anyway.
> > 
> > I'll just take over (1<<26) for MS_NOSYMFOLLOW, and remove the rest in
> > a second patch.
> > 
> > If someone thinks these flags are actually used by something and I'm
> > just missing it, please let me know.
> 
> Afraid you did miss it ...
> 
> /*
>  * sb->s_flags.  Note that these mirror the equivalent MS_* flags where
>  * represented in both.
>  */
> ...
> #define SB_SUBMOUNT     (1<<26)
> 
> It's not entirely clear to me why they need to be the same, but I haven't
> been paying close attention to the separation of superblock and mount
> flags, so someone else can probably explain the why of it.

I could be wrong, but I believe this is historic and originates from the
kernel setting certain flags internally (similar to the whole O_* flag,
"internal" O_* flag, and FMODE_NOTIFY mixup).

Also, one of the arguments for the new mount API was that we'd run out
MS_* bits so it's possible that you have to enable this new mount option
in the new mount API only. (Though Howells is the right person to talk
to on this point.)

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]