[PATCH 0/5] fs: bug fixes

[PATCH 0/5] fs: bug fixes

[Zijun Hu]

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (5):
      fs/filesystems: Fix potential unsigned integer underflow in fs_name()
      fs/fs_parse: Fix macro fsparam_u32hex() definition
      fs/fs_parse: Fix 3 issues for validate_constant_table()
      fs/fs_parse: Correct comments of fs_validate_description()
      fs/fs_context: Mark an unlikely if condition with unlikely() in vfs_parse_monolithic_sep()

 fs/filesystems.c          | 14 +++++++++-----
 fs/fs_context.c           |  2 +-
 fs/fs_parser.c            | 13 ++++++++-----
 include/linux/fs_parser.h |  2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250410-fix_fs-6e0a97c4e59f

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

fs_name() has @index as unsigned int, so there is underflow risk for
operation '@index--'.

Fix by breaking the for loop when '@index == 0' which is also more proper
than '@index <= 0' for unsigned integer comparison.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/filesystems.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 58b9067b2391ce814e580709b518b405e0f9cb8a..95e5256821a53494d88f496193305a2e50e04444 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -156,15 +156,19 @@ static int fs_index(const char __user * __name)
 static int fs_name(unsigned int index, char __user * buf)
 {
 	struct file_system_type * tmp;
-	int len, res;
+	int len, res = -EINVAL;
 
 	read_lock(&file_systems_lock);
-	for (tmp = file_systems; tmp; tmp = tmp->next, index--)
-		if (index <= 0 && try_module_get(tmp->owner))
+	for (tmp = file_systems; tmp; tmp = tmp->next, index--) {
+		if (index == 0) {
+			if (try_module_get(tmp->owner))
+				res = 0;
 			break;
+		}
+	}
 	read_unlock(&file_systems_lock);
-	if (!tmp)
-		return -EINVAL;
+	if (res)
+		return res;
 
 	/* OK, we got the reference, so we can safely block */
 	len = strlen(tmp->name) + 1;

-- 
2.34.1

[PATCH 2/5] fs/fs_parse: Fix macro fsparam_u32hex() definition

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Macro fsparam_u32hex() uses as type @fs_param_is_u32_hex which is
never defined.

Fix by using @fs_param_is_u32 instead as fsparam_u32oct() does.

Fixes: 328de5287b10 ("turn fs_param_is_... into functions")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 include/linux/fs_parser.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 53e566efd5fd133d19e313e494b975612a227b77..ca76601d0bbdbaded76515cb6b2c06fa30127a06 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -126,7 +126,7 @@ static inline bool fs_validate_description(const char *name,
 #define fsparam_u32oct(NAME, OPT) \
 			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)8)
 #define fsparam_u32hex(NAME, OPT) \
-			__fsparam(fs_param_is_u32_hex, NAME, OPT, 0, (void *)16)
+			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)16)
 #define fsparam_s32(NAME, OPT)	__fsparam(fs_param_is_s32, NAME, OPT, 0, NULL)
 #define fsparam_u64(NAME, OPT)	__fsparam(fs_param_is_u64, NAME, OPT, 0, NULL)
 #define fsparam_enum(NAME, OPT, array)	__fsparam(fs_param_is_enum, NAME, OPT, 0, array)

-- 
2.34.1

[PATCH 3/5] fs/fs_parse: Fix 3 issues for validate_constant_table()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Constant table array array[] which must end with a empty entry and fix
below issues for validate_constant_table(array, ARRAY_SIZE(array), ...):

- Always return wrong value for good constant table array which ends
  with a empty entry.

- Imprecise error message for missorted case.

- Potential NULL pointer dereference.

Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/fs_parser.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index e635a81e17d965df78ffef27f6885cd70996c6dd..ef7876340a917876bc40df9cdde9232204125a75 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -399,6 +399,9 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 	}
 
 	for (i = 0; i < tbl_size; i++) {
+		if (!tbl[i].name && (i + 1 == tbl_size))
+			break;
+
 		if (!tbl[i].name) {
 			pr_err("VALIDATE C-TBL[%zu]: Null\n", i);
 			good = false;
@@ -411,13 +414,13 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 				good = false;
 			}
 			if (c > 0) {
-				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>=%s\n",
+				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>%s\n",
 				       i, tbl[i-1].name, tbl[i].name);
 				good = false;
 			}
 		}
 
-		if (tbl[i].value != special &&
+		if (tbl[i].name && tbl[i].value != special &&
 		    (tbl[i].value < low || tbl[i].value > high)) {
 			pr_err("VALIDATE C-TBL[%zu]: %s->%d const out of range (%d-%d)\n",
 			       i, tbl[i].name, tbl[i].value, low, high);

-- 
2.34.1

[PATCH 4/5] fs/fs_parse: Correct comments of fs_validate_description()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For fs_validate_description(), its comments easily mislead reader that
the function will search array @desc for duplicated entries with name
specified by parameter @name, but @name is not used for search actually.

Fix by marking name as owner's name of these parameter specifications.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/fs_parser.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index ef7876340a917876bc40df9cdde9232204125a75..77fd8133c1cf191158de13ec556a5e3c7c2bb12a 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -432,9 +432,9 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
 }
 
 /**
- * fs_validate_description - Validate a parameter description
- * @name: The parameter name to search for.
- * @desc: The parameter description to validate.
+ * fs_validate_description - Validate a parameter specification array
+ * @name: Owner name of the parameter specification array
+ * @desc: The parameter specification array to validate.
  */
 bool fs_validate_description(const char *name,
 	const struct fs_parameter_spec *desc)

-- 
2.34.1

[PATCH 5/5] fs/fs_context: Mark an unlikely if condition with unlikely() in vfs_parse_monolithic_sep()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

There is no mount option with pattern "...,=key_or_value,...", so the if
condition '(value == key)' in while loop of vfs_parse_monolithic_sep() is
is unlikely true.

Mark the condition with unlikely() to improve both performance and
readability.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/fs_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 582d33e8111739402d38dc9fc268e7d14ced3c49..284301d88bc90ef462a08c9ea893f822075a6d4d 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -222,7 +222,7 @@ int vfs_parse_monolithic_sep(struct fs_context *fc, void *data,
 			char *value = strchr(key, '=');
 
 			if (value) {
-				if (value == key)
+				if (unlikely(value == key))
 					continue;
 				*value++ = 0;
 				v_len = strlen(value);

-- 
2.34.1

Re: [PATCH 2/5] fs/fs_parse: Fix macro fsparam_u32hex() definition

[Christian Brauner]

On Thu, Apr 10, 2025 at 07:45:28PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Macro fsparam_u32hex() uses as type @fs_param_is_u32_hex which is
> never defined.
> 
> Fix by using @fs_param_is_u32 instead as fsparam_u32oct() does.
> 
> Fixes: 328de5287b10 ("turn fs_param_is_... into functions")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  include/linux/fs_parser.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index 53e566efd5fd133d19e313e494b975612a227b77..ca76601d0bbdbaded76515cb6b2c06fa30127a06 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -126,7 +126,7 @@ static inline bool fs_validate_description(const char *name,
>  #define fsparam_u32oct(NAME, OPT) \
>  			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)8)
>  #define fsparam_u32hex(NAME, OPT) \
> -			__fsparam(fs_param_is_u32_hex, NAME, OPT, 0, (void *)16)
> +			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)16)

Remove that define completely as it's unused. There's no point keeping
dead code around.

Re: (subset) [PATCH 4/5] fs/fs_parse: Correct comments of fs_validate_description()

[Christian Brauner]

On Thu, 10 Apr 2025 19:45:30 +0800, Zijun Hu wrote:
> For fs_validate_description(), its comments easily mislead reader that
> the function will search array @desc for duplicated entries with name
> specified by parameter @name, but @name is not used for search actually.
> 
> Fix by marking name as owner's name of these parameter specifications.
> 
> 
> [...]

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[4/5] fs/fs_parse: Correct comments of fs_validate_description()
      https://git.kernel.org/vfs/vfs/c/a8fca9b51158

Re: (subset) [PATCH 5/5] fs/fs_context: Mark an unlikely if condition with unlikely() in vfs_parse_monolithic_sep()

[Christian Brauner]

On Thu, 10 Apr 2025 19:45:31 +0800, Zijun Hu wrote:
> There is no mount option with pattern "...,=key_or_value,...", so the if
> condition '(value == key)' in while loop of vfs_parse_monolithic_sep() is
> is unlikely true.
> 
> Mark the condition with unlikely() to improve both performance and
> readability.
> 
> [...]

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[5/5] fs/fs_context: Mark an unlikely if condition with unlikely() in vfs_parse_monolithic_sep()
      https://git.kernel.org/vfs/vfs/c/95bfd4b5928f

Re: [PATCH 2/5] fs/fs_parse: Fix macro fsparam_u32hex() definition

[Zijun Hu]

On 2025/4/11 22:17, Christian Brauner wrote:
>> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
>> index 53e566efd5fd133d19e313e494b975612a227b77..ca76601d0bbdbaded76515cb6b2c06fa30127a06 100644
>> --- a/include/linux/fs_parser.h
>> +++ b/include/linux/fs_parser.h
>> @@ -126,7 +126,7 @@ static inline bool fs_validate_description(const char *name,
>>  #define fsparam_u32oct(NAME, OPT) \
>>  			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)8)
>>  #define fsparam_u32hex(NAME, OPT) \
>> -			__fsparam(fs_param_is_u32_hex, NAME, OPT, 0, (void *)16)
>> +			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)16)
> Remove that define completely as it's unused. There's no point keeping
> dead code around.

sure. will do it. (^^)

Re: (subset) [PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[Christian Brauner]

On Thu, 10 Apr 2025 19:45:27 +0800, Zijun Hu wrote:
> fs_name() has @index as unsigned int, so there is underflow risk for
> operation '@index--'.
> 
> Fix by breaking the for loop when '@index == 0' which is also more proper
> than '@index <= 0' for unsigned integer comparison.
> 
> 
> [...]

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()
      https://git.kernel.org/vfs/vfs/c/d319af11e9a1

Re: [PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[Christian Brauner]

On Thu, Apr 10, 2025 at 07:45:27PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> fs_name() has @index as unsigned int, so there is underflow risk for
> operation '@index--'.
> 
> Fix by breaking the for loop when '@index == 0' which is also more proper
> than '@index <= 0' for unsigned integer comparison.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---

This is honestly not worth the effort thinking about.
I'm going to propose that we remove this system call or at least switch
the default to N. Nobody uses this anymore I'm pretty sure.

>  fs/filesystems.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index 58b9067b2391ce814e580709b518b405e0f9cb8a..95e5256821a53494d88f496193305a2e50e04444 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -156,15 +156,19 @@ static int fs_index(const char __user * __name)
>  static int fs_name(unsigned int index, char __user * buf)
>  {
>  	struct file_system_type * tmp;
> -	int len, res;
> +	int len, res = -EINVAL;
>  
>  	read_lock(&file_systems_lock);
> -	for (tmp = file_systems; tmp; tmp = tmp->next, index--)
> -		if (index <= 0 && try_module_get(tmp->owner))
> +	for (tmp = file_systems; tmp; tmp = tmp->next, index--) {
> +		if (index == 0) {
> +			if (try_module_get(tmp->owner))
> +				res = 0;
>  			break;
> +		}
> +	}
>  	read_unlock(&file_systems_lock);
> -	if (!tmp)
> -		return -EINVAL;
> +	if (res)
> +		return res;
>  
>  	/* OK, we got the reference, so we can safely block */
>  	len = strlen(tmp->name) + 1;
> 
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] fs/fs_parse: Fix 3 issues for validate_constant_table()

[Christian Brauner]

On Thu, Apr 10, 2025 at 07:45:29PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Constant table array array[] which must end with a empty entry and fix
> below issues for validate_constant_table(array, ARRAY_SIZE(array), ...):
> 
> - Always return wrong value for good constant table array which ends
>   with a empty entry.
> 
> - Imprecise error message for missorted case.
> 
> - Potential NULL pointer dereference.

I really dislike "potential NULL deref" without an explanation. Please
explain how this supposed NULL deref can happen.

> Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  fs/fs_parser.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index e635a81e17d965df78ffef27f6885cd70996c6dd..ef7876340a917876bc40df9cdde9232204125a75 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -399,6 +399,9 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
>  	}
>  
>  	for (i = 0; i < tbl_size; i++) {
> +		if (!tbl[i].name && (i + 1 == tbl_size))
> +			break;
> +
>  		if (!tbl[i].name) {
>  			pr_err("VALIDATE C-TBL[%zu]: Null\n", i);
>  			good = false;
> @@ -411,13 +414,13 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
>  				good = false;
>  			}
>  			if (c > 0) {
> -				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>=%s\n",
> +				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>%s\n",
>  				       i, tbl[i-1].name, tbl[i].name);
>  				good = false;
>  			}
>  		}
>  
> -		if (tbl[i].value != special &&
> +		if (tbl[i].name && tbl[i].value != special &&
>  		    (tbl[i].value < low || tbl[i].value > high)) {
>  			pr_err("VALIDATE C-TBL[%zu]: %s->%d const out of range (%d-%d)\n",
>  			       i, tbl[i].name, tbl[i].value, low, high);
> 
> -- 
> 2.34.1
> 

Re: [PATCH 3/5] fs/fs_parse: Fix 3 issues for validate_constant_table()

[Zijun Hu]

On 2025/4/11 22:37, Christian Brauner wrote:
>> - Potential NULL pointer dereference.
> I really dislike "potential NULL deref" without an explanation. Please
> explain how this supposed NULL deref can happen.
> 

okay.

>> Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  fs/fs_parser.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index e635a81e17d965df78ffef27f6885cd70996c6dd..ef7876340a917876bc40df9cdde9232204125a75 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -399,6 +399,9 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
>>  	}
>>  
>>  	for (i = 0; i < tbl_size; i++) {
>> +		if (!tbl[i].name && (i + 1 == tbl_size))
>> +			break;
>> +
>>  		if (!tbl[i].name) {
>>  			pr_err("VALIDATE C-TBL[%zu]: Null\n", i);
>>  			good = false;
>> @@ -411,13 +414,13 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
>>  				good = false;
>>  			}
>>  			if (c > 0) {
>> -				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>=%s\n",
>> +				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>%s\n",
>>  				       i, tbl[i-1].name, tbl[i].name);
>>  				good = false;
>>  			}
>>  		}
>>  
>> -		if (tbl[i].value != special &&
>> +		if (tbl[i].name && tbl[i].value != special &&
>>  		    (tbl[i].value < low || tbl[i].value > high)) {
>>  			pr_err("VALIDATE C-TBL[%zu]: %s->%d const out of range (%d-%d)\n",
>>  			       i, tbl[i].name, tbl[i].value, low, high);

for good constant table which ends with empty entry. for original logic,
when loop reach the last empty entry.  above pr_err() may access NULL
pointer tbl[i].name.


i find out this validate_constant_table() also has no callers.
fix it or remove it ?

Re: [PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[Zijun Hu]

On 2025/4/11 22:35, Christian Brauner wrote:
>> Fix by breaking the for loop when '@index == 0' which is also more proper
>> than '@index <= 0' for unsigned integer comparison.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
> This is honestly not worth the effort thinking about.
> I'm going to propose that we remove this system call or at least switch
> the default to N. Nobody uses this anymore I'm pretty sure

Sound good.

i just started looking at FS code (^^).

Re: [PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[David Howells]

Zijun Hu <zijun_hu@icloud.com> wrote:

> fs_name() has @index as unsigned int, so there is underflow risk for
> operation '@index--'.
> 
> Fix by breaking the for loop when '@index == 0' which is also more proper
> than '@index <= 0' for unsigned integer comparison.

There isn't really a risk.  The list walked by "tmp" and the checks that this
is or is not NULL will prevent a problem.

I also feel that breaking out of the loop with "<= 0" - even if the variable
is unsigned - is safer, on the off chance that someone in the future changes
the signedness of the variable.

David

Re: [PATCH 1/5] fs/filesystems: Fix potential unsigned integer underflow in fs_name()

[Zijun Hu]

On 2025/4/11 23:34, David Howells wrote:
>> Fix by breaking the for loop when '@index == 0' which is also more proper
>> than '@index <= 0' for unsigned integer comparison.
> There isn't really a risk.  The list walked by "tmp" and the checks that this
> is or is not NULL will prevent a problem.
> 

no fixes tag is added and just improve code logic a bit since there is
no reason to continue the loop when @index reach 0.

> I also feel that breaking out of the loop with "<= 0" - even if the variable
> is unsigned - is safer, on the off chance that someone in the future changes
> the signedness of the variable.

for parameter @index representing filesystem index. unsigned integer
type may be better than signed.

Re: [PATCH 3/5] fs/fs_parse: Fix 3 issues for validate_constant_table()

[Jan Kara]

On Fri 11-04-25 22:48:40, Zijun Hu wrote:
> On 2025/4/11 22:37, Christian Brauner wrote:
> >> - Potential NULL pointer dereference.
> > I really dislike "potential NULL deref" without an explanation. Please
> > explain how this supposed NULL deref can happen.
> > 
> 
> okay.
> 
> >> Fixes: 31d921c7fb96 ("vfs: Add configuration parser helpers")
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  fs/fs_parser.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> >> index e635a81e17d965df78ffef27f6885cd70996c6dd..ef7876340a917876bc40df9cdde9232204125a75 100644
> >> --- a/fs/fs_parser.c
> >> +++ b/fs/fs_parser.c
> >> @@ -399,6 +399,9 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
> >>  	}
> >>  
> >>  	for (i = 0; i < tbl_size; i++) {
> >> +		if (!tbl[i].name && (i + 1 == tbl_size))
> >> +			break;
> >> +
> >>  		if (!tbl[i].name) {
> >>  			pr_err("VALIDATE C-TBL[%zu]: Null\n", i);
> >>  			good = false;
> >> @@ -411,13 +414,13 @@ bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
> >>  				good = false;
> >>  			}
> >>  			if (c > 0) {
> >> -				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>=%s\n",
> >> +				pr_err("VALIDATE C-TBL[%zu]: Missorted %s>%s\n",
> >>  				       i, tbl[i-1].name, tbl[i].name);
> >>  				good = false;
> >>  			}
> >>  		}
> >>  
> >> -		if (tbl[i].value != special &&
> >> +		if (tbl[i].name && tbl[i].value != special &&
> >>  		    (tbl[i].value < low || tbl[i].value > high)) {
> >>  			pr_err("VALIDATE C-TBL[%zu]: %s->%d const out of range (%d-%d)\n",
> >>  			       i, tbl[i].name, tbl[i].value, low, high);
> 
> for good constant table which ends with empty entry. for original logic,
> when loop reach the last empty entry.  above pr_err() may access NULL
> pointer tbl[i].name.
> 
> 
> i find out this validate_constant_table() also has no callers.
> fix it or remove it ?

Yeah, just drop it. In the kernel we are pretty aggressive in dropping
unused code :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR