linux-ext4 - Re: [PATCH 02/17] ext4: Add fs parameter description

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191217152012.GY4203@ZenIV.linux.org.uk>
Date:   Tue, 17 Dec 2019 15:20:12 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        David Howells <dhowells@...hat.com>
Subject: Re: [PATCH 02/17] ext4: Add fs parameter description

On Tue, Dec 17, 2019 at 01:19:56PM +0100, Lukas Czerner wrote:
> On Tue, Dec 17, 2019 at 12:44:19AM +0000, Al Viro wrote:
> > On Wed, Nov 06, 2019 at 11:14:42AM +0100, Lukas Czerner wrote:
> > > +	fsparam_string_empty
> > > +			("usrjquota",		Opt_usrjquota),
> > > +	fsparam_string_empty
> > > +			("grpjquota",		Opt_grpjquota),
> > 
> > Umm...  That makes ...,usrjquota,... equivalent to ...,usrjquota=,...
> > unless I'm misreading the series.  Different from mainline, right?
> 
> Unfortunatelly yes, I do not think this is a problem, but if you have a
> solution within the new mount api framework I am happy to use it.

Er...  Dump the fsparam_string_empty() use and instead of your
+       if (token == Opt_usrjquota) {
+               if (result.negated)
+                       return clear_qf_name(sb, USRQUOTA);
+               else
+                       return set_qf_name(sb, USRQUOTA, param);
do
+       if (token == Opt_usrjquota) {
+               if (!param->string[0])
+                       return clear_qf_name(sb, USRQUOTA);
+               else
+                       return set_qf_name(sb, USRQUOTA, param);
with the same for grpjquota?

> > > +	fsparam_bool	("barrier",		Opt_barrier),
> > > +	fsparam_flag	("nobarrier",		Opt_nobarrier),
> > 
> > That's even more interesting.  Current mainline:
> > 		barrier		OK, sets EXT4_MOUNT_BARRIER
> > 		barrier=0	OK, sets EXT4_MOUNT_BARRIER
> > 		barrier=42	OK, sets EXT4_MOUNT_BARRIER
> > 		barrier=yes	error
> > 		barrier=no	error
> > 		nobarrier	OK, clears EXT4_MOUNT_BARRIER
> > Unless I'm misreading your series, you get
> > 		barrier		error
> 
> Not really, this seems to be working as expected. Assuming that this
> didn't change since 5.4.0-rc6. I does make sense to me that specifying
> bool type parameter without any options would express "true".
> 
> 
> > 		barrier=0	OK, sets EXT4_MOUNT_BARRIER
> 
> 
> > 		barrier=42	error
> > 		barrier=yes	OK, sets EXT4_MOUNT_BARRIER
> > 		barrier=no	OK, sets EXT4_MOUNT_BARRIER
> 
> Those three are different, just because of how param_book() work. I do
> not really see a problem with it, but if we want to keep it exactly the
> same as current mainline it would be difficult with how the current api
> works. Any suggestions ?

If fsparam_bool() doesn't do the right thing, perhaps it shouldn't be
used in the first place?  Or changed, for that matter - it's not as if
we had many users of that thing and the only in-tree one is definitely
breaking userland ABI.

In case of ext4, after rereading that code (and getting some sleep) the
current behaviour is, AFAICS to accept barrier | nobarrier | barrier=<number>
with the last case being equialent to nobarrier when number is 0 and barrier
in all other cases.  Is that an accurate description?

If so, I would prefer
	fsparam_flag_no("barrier", Opt_barrier),	// barrier | nobarrier
	fsparam_u32("barrier", Opt_barrier),		// barrier=<number>
as the solution, with fs_parse() having been taught to allow argument-bearing
and argument-less options with the same name, picking the right one.  That
way Opt_nobarrier gets removed as well...

I'll push a branch with that stuff later today; will post when it's out...