[PATCH] erofs: reject unknown option if it is not supported

[PATCH] erofs: reject unknown option if it is not supported

[Hongbo Li]

Some options are supported depending on different compiling config,
and these option will not fail during mount if they are not
supported. This is very weird, so we can reject them if they are
not supported.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/erofs/super.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index cadec6b1b554..c1c350c6fbf4 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -374,16 +374,26 @@ static const struct constant_table erofs_dax_param_enums[] = {
 };
 
 static const struct fs_parameter_spec erofs_fs_parameters[] = {
+#ifdef CONFIG_EROFS_FS_XATTR
 	fsparam_flag_no("user_xattr",	Opt_user_xattr),
+#endif
+#ifdef CONFIG_EROFS_FS_POSIX_ACL
 	fsparam_flag_no("acl",		Opt_acl),
+#endif
+#ifdef CONFIG_EROFS_FS_ZIP
 	fsparam_enum("cache_strategy",	Opt_cache_strategy,
 		     erofs_param_cache_strategy),
+#endif
 	fsparam_flag("dax",             Opt_dax),
 	fsparam_enum("dax",		Opt_dax_enum, erofs_dax_param_enums),
 	fsparam_string("device",	Opt_device),
+#ifdef CONFIG_EROFS_FS_ONDEMAND
 	fsparam_string("fsid",		Opt_fsid),
 	fsparam_string("domain_id",	Opt_domain_id),
+#endif
+#ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
 	fsparam_flag_no("directio",	Opt_directio),
+#endif
 	{}
 };
 
@@ -424,33 +434,27 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		return opt;
 
 	switch (opt) {
-	case Opt_user_xattr:
 #ifdef CONFIG_EROFS_FS_XATTR
+	case Opt_user_xattr:
 		if (result.boolean)
 			set_opt(&sbi->opt, XATTR_USER);
 		else
 			clear_opt(&sbi->opt, XATTR_USER);
-#else
-		errorfc(fc, "{,no}user_xattr options not supported");
-#endif
 		break;
-	case Opt_acl:
+#endif
 #ifdef CONFIG_EROFS_FS_POSIX_ACL
+	case Opt_acl:
 		if (result.boolean)
 			set_opt(&sbi->opt, POSIX_ACL);
 		else
 			clear_opt(&sbi->opt, POSIX_ACL);
-#else
-		errorfc(fc, "{,no}acl options not supported");
-#endif
 		break;
-	case Opt_cache_strategy:
+#endif
 #ifdef CONFIG_EROFS_FS_ZIP
+	case Opt_cache_strategy:
 		sbi->opt.cache_strategy = result.uint_32;
-#else
-		errorfc(fc, "compression not supported, cache_strategy ignored");
-#endif
 		break;
+#endif
 	case Opt_dax:
 		if (!erofs_fc_set_dax_mode(fc, EROFS_MOUNT_DAX_ALWAYS))
 			return -EINVAL;
@@ -491,22 +495,15 @@ static int erofs_fc_parse_param(struct fs_context *fc,
 		if (!sbi->domain_id)
 			return -ENOMEM;
 		break;
-#else
-	case Opt_fsid:
-	case Opt_domain_id:
-		errorfc(fc, "%s option not supported", erofs_fs_parameters[opt].name);
-		break;
 #endif
-	case Opt_directio:
 #ifdef CONFIG_EROFS_FS_BACKED_BY_FILE
+	case Opt_directio:
 		if (result.boolean)
 			set_opt(&sbi->opt, DIRECT_IO);
 		else
 			clear_opt(&sbi->opt, DIRECT_IO);
-#else
-		errorfc(fc, "%s option not supported", erofs_fs_parameters[opt].name);
-#endif
 		break;
+#endif
 	}
 	return 0;
 }
-- 
2.22.0

Re: [PATCH] erofs: reject unknown option if it is not supported

[Gao Xiang]

On Mon, Apr 28, 2025 at 02:25:45PM +0000, Hongbo Li wrote:
> Some options are supported depending on different compiling config,
> and these option will not fail during mount if they are not
> supported. This is very weird, so we can reject them if they are
> not supported.
> 

If it's an invalid option, we should reject it immediately.

But for unsupported options, I don't think we always error
out. e.g. for some options like (acl, noacl) ext4 will just
ignore if ACL is unsupported.

I think EROFS should follows that, otherwise users might use
"noacl" to disable ACL explicitly, but it will fail unexpectedly
if unsupported.

But I agree that for "fsid", "domain_id" and "directio", we
could error out instead.

> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>  fs/erofs/super.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index cadec6b1b554..c1c350c6fbf4 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -374,16 +374,26 @@ static const struct constant_table erofs_dax_param_enums[] = {
>  };
>  
>  static const struct fs_parameter_spec erofs_fs_parameters[] = {
> +#ifdef CONFIG_EROFS_FS_XATTR
>  	fsparam_flag_no("user_xattr",	Opt_user_xattr),
> +#endif

Another thing is that I'm not sure if "user_xattr" option is really
needed, we might just kill this option since all recent fses don't
have such configuration and user_xattrs should be supported by default.

Thanks,
Gao Xiang

Re: [PATCH] erofs: reject unknown option if it is not supported

[Hongbo Li]

On 2025/4/28 23:16, Gao Xiang wrote:
> On Mon, Apr 28, 2025 at 02:25:45PM +0000, Hongbo Li wrote:
>> Some options are supported depending on different compiling config,
>> and these option will not fail during mount if they are not
>> supported. This is very weird, so we can reject them if they are
>> not supported.
>>
> 
> If it's an invalid option, we should reject it immediately.
> 
> But for unsupported options, I don't think we always error
> out. e.g. for some options like (acl, noacl) ext4 will just
> ignore if ACL is unsupported.
> 
Thanks for reviewing!
I will keep this in later version.

> I think EROFS should follows that, otherwise users might use
> "noacl" to disable ACL explicitly, but it will fail unexpectedly
> if unsupported.
> 
> But I agree that for "fsid", "domain_id" and "directio", we
> could error out instead.
> 
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>> ---
>>   fs/erofs/super.c | 39 ++++++++++++++++++---------------------
>>   1 file changed, 18 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>> index cadec6b1b554..c1c350c6fbf4 100644
>> --- a/fs/erofs/super.c
>> +++ b/fs/erofs/super.c
>> @@ -374,16 +374,26 @@ static const struct constant_table erofs_dax_param_enums[] = {
>>   };
>>   
>>   static const struct fs_parameter_spec erofs_fs_parameters[] = {
>> +#ifdef CONFIG_EROFS_FS_XATTR
>>   	fsparam_flag_no("user_xattr",	Opt_user_xattr),
>> +#endif
> 
> Another thing is that I'm not sure if "user_xattr" option is really
> needed, we might just kill this option since all recent fses don't
> have such configuration and user_xattrs should be supported by default.
> 
Yeah, perhaps this option should be removed along with 
CONFIG_EROFS_FS_XATTR, as xattr can be also consider as a type of data 
that we cannot modify.

Thanks,
Hongbo

> Thanks,
> Gao Xiang
> 

Re: [PATCH] erofs: reject unknown option if it is not supported

[Gao Xiang]

On Tue, Apr 29, 2025 at 11:46:39AM +0800, Hongbo Li wrote:

...

> > 
> > Another thing is that I'm not sure if "user_xattr" option is really
> > needed, we might just kill this option since all recent fses don't
> > have such configuration and user_xattrs should be supported by default.
> > 
> Yeah, perhaps this option should be removed along with
> CONFIG_EROFS_FS_XATTR, as xattr can be also consider as a type of data that
> we cannot modify.

You meant remove "CONFIG_EROFS_FS_XATTR"? but some embedded
use cases don't need xattrs anyway.

I mean just kill "user_xattr" option and leave user xattrs on
all the time.

Thanks,
Gao Xiang

Re: [PATCH] erofs: reject unknown option if it is not supported

[Hongbo Li]

On 2025/4/29 11:54, Gao Xiang wrote:
> On Tue, Apr 29, 2025 at 11:46:39AM +0800, Hongbo Li wrote:
> 
> ...
> 
>>>
>>> Another thing is that I'm not sure if "user_xattr" option is really
>>> needed, we might just kill this option since all recent fses don't
>>> have such configuration and user_xattrs should be supported by default.
>>>
>> Yeah, perhaps this option should be removed along with
>> CONFIG_EROFS_FS_XATTR, as xattr can be also consider as a type of data that
>> we cannot modify.
> 
> You meant remove "CONFIG_EROFS_FS_XATTR"? but some embedded
> use cases don't need xattrs anyway.
> 
> I mean just kill "user_xattr" option and leave user xattrs on
> all the time.
ok, it seems reasonable.

Thanks,
Hongbo

> 
> Thanks,
> Gao Xiang
>