Re: mount on tmpfs failing to parse context option

[Ian Kent]

On Tue, 2019-10-08 at 20:38 +0800, Ian Kent wrote:
> On Mon, 2019-10-07 at 17:50 -0700, Hugh Dickins wrote:
> > On Mon, 7 Oct 2019, Laura Abbott wrote:
> > > On 9/30/19 12:07 PM, Laura Abbott wrote:
> > > > Hi,
> > > > 
> > > > Fedora got a bug report 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1757104
> > > > of a failure to parse options with the context mount option.
> > > > From
> > the
> > > > reporter:
> > > > 
> > > > 
> > > > $ unshare -rm mount -t tmpfs tmpfs /tmp -o
> > > > 'context="system_u:object_r:container_file_t:s0:c475,c690"'
> > > > mount: /tmp: wrong fs type, bad option, bad superblock on
> > > > tmpfs,
> > missing
> > > > codepage or helper program, or other error.
> > > > 
> > > > 
> > > > Sep 30 16:50:42 kernel: tmpfs: Unknown parameter 'c690"'
> > > > 
> > > > I haven't asked the reporter to bisect yet but I'm suspecting
> > > > one
> > of the
> > > > conversion to the new mount API:
> > > > 
> > > > $ git log --oneline v5.3..origin/master mm/shmem.c
> > > > edf445ad7c8d Merge branch 'hugepage-fallbacks' (hugepatch
> > > > patches
> > from
> > > > David Rientjes)
> > > > 19deb7695e07 Revert "Revert "Revert "mm, thp: consolidate THP
> > > > gfp
> > handling
> > > > into alloc_hugepage_direct_gfpmask""
> > > > 28eb3c808719 shmem: fix obsolete comment in shmem_getpage_gfp()
> > > > 4101196b19d7 mm: page cache: store only head pages in i_pages
> > > > d8c6546b1aea mm: introduce compound_nr()
> > > > f32356261d44 vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs
> > to use the
> > > > new mount API
> > > > 626c3920aeb4 shmem_parse_one(): switch to use of fs_parse()
> > > > e04dc423ae2c shmem_parse_options(): take handling a single
> > > > option
> > into a
> > > > helper
> > > > f6490b7fbb82 shmem_parse_options(): don't bother with mpol in
> > separate
> > > > variable
> > > > 0b5071dd323d shmem_parse_options(): use a separate structure to
> > keep the
> > > > results
> > > > 7e30d2a5eb0b make shmem_fill_super() static
> > > > 
> > > > 
> > > > I didn't find another report or a fix yet. Is it worth asking
> > > > the
> > reporter
> > > > to bisect?
> > > > 
> > > > Thanks,
> > > > Laura
> > > 
> > > Ping again, I never heard anything back and I didn't see anything
> > come in
> > > with -rc2
> > 
> > Sorry for not responding sooner, Laura, I was travelling: and
> > dearly
> > hoping that David or Al would take it.  I'm afraid this is rather
> > beyond
> > my capability (can I admit that it's the first time I even heard of
> > the
> > "context" mount option? and grepping for "context" has not yet
> > shown
> > me
> > at what level it is handled; and I've no idea of what a valid
> > "context"
> > is for my own tmpfs mounts, to start playing around with its
> > parsing).
> > 
> > Yes, I think we can assume that this bug comes from f32356261d44
> > ("vfs:
> > Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount
> > API")
> > or one of shmem_parse ones associated with it; but I'm pretty sure
> > that
> > it's not worth troubling the reporter to bisect.  I expect David
> > and
> > Al
> > are familiar with "context", and can go straight to where it's
> > handled,
> > and see what's up.
> > 
> > (tmpfs, very tiresomely, supports a NUMA "mpol" mount option which
> > can
> > have commas in it e.g "mpol=bind:0,2": which makes all its comma
> > parsing
> > awkward.  I assume that where the new mount API commits bend over
> > to
> > accommodate that peculiarity, they end up mishandling the comma in
> > the context string above.)
> > 
> > And since we're on the subject of new mount API breakage in tmpfs,
> > I'll
> > take the liberty of repeating this different case, reported earlier
> > and
> > still broken in rc2: again something that I'd be hard-pressed to
> > fix
> > myself, without endangering some other filesystem's mount parsing:-
> > 
> > My /etc/fstab has a line in for one of my test mounts:
> > tmpfs                /tlo                 tmpfs     
> > size=4G               0 0
> > and that "size=4G" is what causes the problem: because each time
> > shmem_parse_options(fc, data) is called for a remount, data (that
> > is,
> > options) points to a string starting with "size=4G,", followed by
> > what's actually been asked for in the remount options.
> > 
> > So if I try
> > mount -o remount,size=0 /tlo
> > that succeeds, setting the filesystem size to 0 meaning unlimited.
> > So if then as a test I try
> > mount -o remount,size=1M /tlo
> > that correctly fails with "Cannot retroactively limit size".
> > But then when I try
> > mount -o remount,nr_inodes=0 /tlo
> > I again get "Cannot retroactively limit size",
> > when it should have succeeded (again, 0 here meaning unlimited).
> > 
> > That's because the options in shmem_parse_options() are
> > "size=4G,nr_inodes=0", which indeed looks like an attempt to
> > retroactively limit size; but the user never asked "size=4G" there.
> 
> I believe that's mount(8) doing that.
> I don't think it's specific to the new mount api.
> 
> AFAIK it's not new but it does mean the that things that come
> through that have been found in mtab by mount(8) need to be
> checked against the current value before failing or ignored if
> changing them is not allowed.
> 
> I wonder if the problem has been present for quite a while but
> gone unnoticed perhaps.
> 
> IIUC the order should always be command line options last and it
> must be that way to honour the last specified option takes
> precedence convention.
> 
> I thought this was well known, but maybe I'm wrong ... and TBH
> I wasn't aware of it until recently myself.

And it occurs to be that using the same working storage (eg.
ctx->blocks) for more than one option is a problem too.

Even if those options are mutually exclusive the options of
the current mount feed in by mount(8) shouldn't cause the
mount to fail.

Also, and the bit that is mount api specific, the parameter
parsing is done before calling reconfigure so it can't check
if the option is current at that time.

Ian