Re: mount on tmpfs failing to parse context option
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
Re: mount on tmpfs failing to parse context option
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
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 http://karelzak.blogspot.com
Re: mount on tmpfs failing to parse context option
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. > > A
Re: mount on tmpfs failing to parse context option
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
Re: mount on tmpfs failing to parse context option
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
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
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
mount on tmpfs failing to parse context option
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