Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <cf9a666c-e663-fb9e-ba3f-4052edf17536@themaw.net>
Date:   Tue, 18 Oct 2022 10:07:13 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [REPOST PATCH v3 2/2] vfs: parse: deal with zero length string
 value
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Al Viro <viro@ZenIV.linux.org.uk>,
        Siddhesh Poyarekar <siddhesh@gotplt.org>,
        David Howells <dhowells@redhat.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Carlos Maiolino <cmaiolino@redhat.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <166365872189.39016.10771273319597352356.stgit@donald.themaw.net>
 <166365878918.39016.12757946948158123324.stgit@donald.themaw.net>
 <20221017185523.22f43b5d7f9fee1e1e3d872f@linux-foundation.org>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <20221017185523.22f43b5d7f9fee1e1e3d872f@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 18/10/22 09:55, Andrew Morton wrote:
> On Tue, 20 Sep 2022 15:26:29 +0800 Ian Kent <raven@themaw.net> wrote:
>
>> Parsing an fs string that has zero length should result in the parameter
>> being set to NULL so that downstream processing handles it correctly.
>> For example, the proc mount table processing should print "(none)" in
>> this case to preserve mount record field count, but if the value points
>> to the NULL string this doesn't happen.
>>
>> ...
>>
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -197,6 +197,8 @@ int fs_param_is_bool(struct p_log *log, const struct fs_parameter_spec *p,
>>   		     struct fs_parameter *param, struct fs_parse_result *result)
>>   {
>>   	int b;
>> +	if (param->type == fs_value_is_empty)
>> +		return 0;
>>   	if (param->type != fs_value_is_string)
>>   		return fs_param_bad_value(log, param);
>>   	if (!*param->string && (p->flags & fs_param_can_be_empty))
>> @@ -213,6 +215,8 @@ int fs_param_is_u32(struct p_log *log, const struct fs_parameter_spec *p,
>>   		    struct fs_parameter *param, struct fs_parse_result *result)
>>   {
>>   	int base = (unsigned long)p->data;
>> +	if (param->type == fs_value_is_empty)
>> +		return 0;
>>   	if (param->type != fs_value_is_string)
>>   		return fs_param_bad_value(log, param);
>>   	if (!*param->string && (p->flags & fs_param_can_be_empty))
>>
>> [etcetera]
> This feels wrong.  Having to check for fs_value_is_empty in so many
> places makes me think "we just shouldn't have got this far".  Am I
> right for once?


Maybe, did you have a different approach in mind ... ?


My thought was that these helper functions need to protect themselves

against things that could creep in and we don't know what they may be.


I'm not sure the best strategy is to not ever do this type of call in 
the first

place ...


Ian