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]

Date:   Tue, 17 Dec 2019 17:34:32 +0100
From:   Lukas Czerner <lczerner@...hat.com>
To:     Al Viro <viro@...iv.linux.org.uk>
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 03:20:12PM +0000, Al Viro wrote:
> 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?

Ah right, it's been a while I guess I didn't realize that it will accept
usrjquota as well as usrjquota=

It makes sense to check the string directly, thanks.

> 
> > > > +	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?

Yeah, the documentation says
barrier=0 / barrier=1 / barrier / nobarrier

but we do accept any number from 0 to 2147483647

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

That would be great, thanks.

-Lukas