Re: mount on tmpfs failing to parse context option

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

Re: mount on tmpfs failing to parse context option

[Al Viro]

On Mon, Oct 07, 2019 at 05:50:31PM -0700, Hugh Dickins wrote:

[sorry for being MIA - had been sick through the last week, just digging
myself from under piles of mail; my apologies]

> (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.)

	Dumber than that, I'm afraid.  mpol is the reason for having
->parse_monolithic() in the first place, all right, but the problem is
simply the lack of security_sb_eat_lsm_opts() call in it.

	Could you check if the following fixes that one?

diff --git a/mm/shmem.c b/mm/shmem.c
index 0f7fd4a85db6..8dcc8d04cbaf 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3482,6 +3482,12 @@ static int shmem_parse_options(struct fs_context *fc, void *data)
 {
 	char *options = data;
 
+	if (options) {
+		int err = security_sb_eat_lsm_opts(options, &fc->security);
+		if (err)
+			return err;
+	}
+
 	while (options != NULL) {
 		char *this_char = options;
 		for (;;) {

Re: mount on tmpfs failing to parse context option

[Laura Abbott]

On 10/7/19 9:26 PM, Al Viro wrote:
> On Mon, Oct 07, 2019 at 05:50:31PM -0700, Hugh Dickins wrote:
> 
> [sorry for being MIA - had been sick through the last week, just digging
> myself from under piles of mail; my apologies]
> 
>> (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.)
> 
> 	Dumber than that, I'm afraid.  mpol is the reason for having
> ->parse_monolithic() in the first place, all right, but the problem is
> simply the lack of security_sb_eat_lsm_opts() call in it.
> 
> 	Could you check if the following fixes that one?
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 0f7fd4a85db6..8dcc8d04cbaf 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3482,6 +3482,12 @@ static int shmem_parse_options(struct fs_context *fc, void *data)
>   {
>   	char *options = data;
>   
> +	if (options) {
> +		int err = security_sb_eat_lsm_opts(options, &fc->security);
> +		if (err)
> +			return err;
> +	}
> +
>   	while (options != NULL) {
>   		char *this_char = options;
>   		for (;;) {
> 

Yes the reporter says that works.

Thanks,
Laura

Re: mount on tmpfs failing to parse context option

[Ian Kent]

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.

> 
> Hugh

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

Re: mount on tmpfs failing to parse context option

[Karel Zak]

On Tue, Oct 08, 2019 at 08:38:18PM +0800, Ian Kent wrote:
> > 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.

Yep, the common behavior is "the last option wins". See man mount,
remount option:

  remount  functionality  follows  the standard way the mount command
  works with options from fstab.  This means that mount does not read
  fstab (or mtab) only when both device and dir are specified.

        mount -o remount,rw /dev/foo /dir

  After this call all old mount options are replaced and arbitrary
  stuff from fstab (or mtab) is ignored, except the loop= option which
  is  internally  generated  and  maintained by the mount command.

        mount -o remount,rw  /dir

  After  this call, mount reads fstab and merges these options with
  the options from the command line (-o).  If no mountpoint is found
  in fstab, then a remount with unspeci‐ fied source is allowed.


If you do not like this classic behavior than recent mount(8) versions
provide --options-mode={ignore,append,prepend,replace} to keep it in
your hands.


    Karel

> 
> > 
> > Hugh
> 

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: mount on tmpfs failing to parse context option

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3048 bytes --]

On Tue, 8 Oct 2019, Karel Zak wrote:
> On Tue, Oct 08, 2019 at 08:38:18PM +0800, Ian Kent wrote:
> > > 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.
> 
> Yep, the common behavior is "the last option wins". See man mount,
> remount option:
> 
>   remount  functionality  follows  the standard way the mount command
>   works with options from fstab.  This means that mount does not read
>   fstab (or mtab) only when both device and dir are specified.
> 
>         mount -o remount,rw /dev/foo /dir
> 
>   After this call all old mount options are replaced and arbitrary
>   stuff from fstab (or mtab) is ignored, except the loop= option which
>   is  internally  generated  and  maintained by the mount command.
> 
>         mount -o remount,rw  /dir
> 
>   After  this call, mount reads fstab and merges these options with
>   the options from the command line (-o).  If no mountpoint is found
>   in fstab, then a remount with unspeci‐ fied source is allowed.
> 
> 
> If you do not like this classic behavior than recent mount(8) versions
> provide --options-mode={ignore,append,prepend,replace} to keep it in
> your hands.

Ian, Karel, many thanks for your very helpful education.
I've not yet digested all of it, but the important thing is...

Yes, you're right: my unexpectedly failing remount sequence fails
equally on a v5.3 kernel, and I'll hazard a guess that it has failed
like that ever since v2.4.8.  I just never noticed (and nobody else
ever complained) until I tried testing the new mount API: which at
least has the courtesy to put an error message reflecting the final
decision in dmesg, when the older kernels just silently EINVALed.

(And it's not impossible to remount thereafter: one just has to add
a "size=0" into the options, to allow the other options through.)

So, I've no more worries for v5.4 tmpfs mount, and if there's anything
that can be improved, that's a background job for me to look into later,
once I've spent more time understanding the info you've given me.

And Laura has confirmed that Al's security_sb_eat_lsm_opts() patch
fixes the "context" issue: thanks.

Hugh