Received-SPF: pass (google.com: domain of linux-kernel+bounces-96776-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 8 Mar 2024 10:53:25 +0100
From: Christian Brauner <brauner@kernel.org>
To: Jan Kara <jack@suse.cz>
Cc: Luis Henriques <lhenriques@suse.de>, Theodore Ts'o <tytso@mit.edu>, 
	Andreas Dilger <adilger.kernel@dilger.ca>, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Miklos Szeredi <miklos@szeredi.hu>, Amir Goldstein <amir73il@gmail.com>, linux-ext4@vger.kernel.org, 
	linux-fsdevel@vger.kernel.org, linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] fs_parser: handle parameters that can be empty and
 don't have a value
Message-ID: <20240308-fahrdienst-torten-eae8f3eed3b4@brauner>
References: <20240229163011.16248-1-lhenriques@suse.de>
 <20240229163011.16248-2-lhenriques@suse.de>
 <20240301-gegossen-seestern-683681ea75d1@brauner>
 <87il269crs.fsf@suse.de>
 <20240307151356.ishrtxrsge2i5mjn@quack3>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240307151356.ishrtxrsge2i5mjn@quack3>

On Thu, Mar 07, 2024 at 04:13:56PM +0100, Jan Kara wrote:
> On Fri 01-03-24 15:45:27, Luis Henriques wrote:
> > Christian Brauner <brauner@kernel.org> writes:
> > 
> > > On Thu, Feb 29, 2024 at 04:30:08PM +0000, Luis Henriques wrote:
> > >> Currently, only parameters that have the fs_parameter_spec 'type' set to
> > >> NULL are handled as 'flag' types.  However, parameters that have the
> > >> 'fs_param_can_be_empty' flag set and their value is NULL should also be
> > >> handled as 'flag' type, as their type is set to 'fs_value_is_flag'.
> > >> 
> > >> Signed-off-by: Luis Henriques <lhenriques@suse.de>
> > >> ---
> > >>  fs/fs_parser.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> > >> index edb3712dcfa5..53f6cb98a3e0 100644
> > >> --- a/fs/fs_parser.c
> > >> +++ b/fs/fs_parser.c
> > >> @@ -119,7 +119,8 @@ int __fs_parse(struct p_log *log,
> > >>  	/* Try to turn the type we were given into the type desired by the
> > >>  	 * parameter and give an error if we can't.
> > >>  	 */
> > >> -	if (is_flag(p)) {
> > >> +	if (is_flag(p) ||
> > >> +	    (!param->string && (p->flags & fs_param_can_be_empty))) {
> > >>  		if (param->type != fs_value_is_flag)
> > >>  			return inval_plog(log, "Unexpected value for '%s'",
> > >>  				      param->key);
> > >
> > > If the parameter was derived from FSCONFIG_SET_STRING in fsconfig() then
> > > param->string is guaranteed to not be NULL. So really this is only
> > > about:
> > >
> > > FSCONFIG_SET_FD
> > > FSCONFIG_SET_BINARY
> > > FSCONFIG_SET_PATH
> > > FSCONFIG_SET_PATH_EMPTY
> > >
> > > and those values being used without a value. What filesystem does this?
> > > I don't see any.
> > >
> > > The tempting thing to do here is to to just remove fs_param_can_be_empty
> > > from every helper that isn't fs_param_is_string() until we actually have
> > > a filesystem that wants to use any of the above as flags. Will lose a
> > > lot of code that isn't currently used.
> > 
> > Right, I find it quite confusing and I may be fixing the issue in the
> > wrong place.  What I'm seeing with ext4 when I mount a filesystem using
> > the option '-o usrjquota' is that fs_parse() will get:
> > 
> >  * p->type is set to fs_param_is_string
> >    ('p' is a struct fs_parameter_spec, ->type is a function)
> >  * param->type is set to fs_value_is_flag
> >    ('param' is a struct fs_parameter, ->type is an enum)
> > 
> > This is because ext4 will use the __fsparam macro to set define a
> > fs_param_spec as a fs_param_is_string but will also set the
> > fs_param_can_be_empty; and the fsconfig() syscall will get that parameter
> > as a flag.  That's why param->string will be NULL in this case.
> 
> So I'm a bit confused here. Valid variants of these quota options are like
> "usrjquota=<filename>" (to set quota file name) or "usrjquota=" (to clear
> quota file name). The variant "usrjquota" should ideally be rejected
> because it doesn't make a good sense and only adds to confusion. Now as far
> as I'm reading fs/ext4/super.c: parse_options() (and as far as my testing
> shows) this is what is happening so what is exactly the problem you're
> trying to fix?

mount(8) has no way of easily knowing that for something like
mount -o usrjquota /dev/sda1 /mnt that "usrjquota" is supposed to be
set as an empty string via FSCONFIG_SET_STRING. For mount(8) it is
indistinguishable from a flag because it's specified without an
argument. So mount(8) passes FSCONFIG_SET_FLAG and it seems strange that
we should require mount(8) to know what mount options are strings or no.
I've ran into this issue before myself when using the mount api
programatically.