Re: mount on tmpfs failing to parse context option

[Hugh Dickins]

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.

Hugh