Re: [PATCH v4] fs/fs_parse: Remove unused and problematic validate_constant_table()

[Jan Kara]

On Tue 15-04-25 20:25:00, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> 
> Remove validate_constant_table() since:
> 
> - It has no caller.
> 
> - It has below 3 bugs for good constant table array array[] which must
>   end with a empty entry, and take below invocation for explaination:
>   validate_constant_table(array, ARRAY_SIZE(array), ...)
> 
>   - Always return wrong value due to the last empty entry.
>   - Imprecise error message for missorted case.
>   - Potential NULL pointer dereference since the last pr_err() may use
>     @tbl[i].name NULL pointer to print the last empty entry's name.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Changes in v4:
> - Rebase on vfs-6.16.misc branch to fix merge conflict.
> - Link to v3: https://lore.kernel.org/r/20250415-fix_fs-v3-1-0c378cc5ce35@xxxxxxxxxxx
> 
> Changes in v3:
> - Remove validate_constant_table() instead of fixing it
> - Link to v2: https://lore.kernel.org/r/20250411-fix_fs-v2-2-5d3395c102e4@xxxxxxxxxxx
> 
> Changes in v2:
> - Remove fixes tag for remaining patches
> - Add more comment for the NULL pointer dereference issue
> - Link to v1: https://lore.kernel.org/r/20250410-fix_fs-v1-3-7c14ccc8ebaa@xxxxxxxxxxx
> ---
>  Documentation/filesystems/mount_api.rst | 15 ----------
>  fs/fs_parser.c                          | 49 ---------------------------------
>  include/linux/fs_parser.h               |  5 ----
>  3 files changed, 69 deletions(-)
> 
> diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
> index 47dafbb7427e6a829989a815e4d034e48fdbe7a2..e149b89118c885c377a17b95adcdbcb594b34e00 100644
> --- a/Documentation/filesystems/mount_api.rst
> +++ b/Documentation/filesystems/mount_api.rst
> @@ -752,21 +752,6 @@ process the parameters it is given.
>       If a match is found, the corresponding value is returned.  If a match
>       isn't found, the not_found value is returned instead.
>  
> -   * ::
> -
> -       bool validate_constant_table(const struct constant_table *tbl,
> -				    size_t tbl_size,
> -				    int low, int high, int special);
> -
> -     Validate a constant table.  Checks that all the elements are appropriately
> -     ordered, that there are no duplicates and that the values are between low
> -     and high inclusive, though provision is made for one allowable special
> -     value outside of that range.  If no special value is required, special
> -     should just be set to lie inside the low-to-high range.
> -
> -     If all is good, true is returned.  If the table is invalid, errors are
> -     logged to the kernel log buffer and false is returned.
> -
>     * ::
>  
>         bool fs_validate_description(const char *name,
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index c5cb19788f74771a945801ceedeec69efed0e40a..c092a9f79e324bacbd950165a0eb66632cae9e03 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -379,55 +379,6 @@ int fs_param_is_path(struct p_log *log, const struct fs_parameter_spec *p,
>  EXPORT_SYMBOL(fs_param_is_path);
>  
>  #ifdef CONFIG_VALIDATE_FS_PARSER
> -/**
> - * validate_constant_table - Validate a constant table
> - * @tbl: The constant table to validate.
> - * @tbl_size: The size of the table.
> - * @low: The lowest permissible value.
> - * @high: The highest permissible value.
> - * @special: One special permissible value outside of the range.
> - */
> -bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
> -			     int low, int high, int special)
> -{
> -	size_t i;
> -	bool good = true;
> -
> -	if (tbl_size == 0) {
> -		pr_warn("VALIDATE C-TBL: Empty\n");
> -		return true;
> -	}
> -
> -	for (i = 0; i < tbl_size; i++) {
> -		if (!tbl[i].name) {
> -			pr_err("VALIDATE C-TBL[%zu]: Null\n", i);
> -			good = false;
> -		} else if (i > 0 && tbl[i - 1].name) {
> -			int c = strcmp(tbl[i-1].name, tbl[i].name);
> -
> -			if (c == 0) {
> -				pr_err("VALIDATE C-TBL[%zu]: Duplicate %s\n",
> -				       i, tbl[i].name);
> -				good = false;
> -			}
> -			if (c > 0) {
> -				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 &&
> -		    (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);
> -			good = false;
> -		}
> -	}
> -
> -	return good;
> -}
> -
>  /**
>   * fs_validate_description - Validate a parameter specification array
>   * @name: Owner name of the parameter specification array
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index 5057faf4f09182fa6e7ddd03fb17b066efd7e58b..5a0e897cae807bbf5472645735027883a6593e91 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -87,14 +87,9 @@ extern int lookup_constant(const struct constant_table tbl[], const char *name,
>  extern const struct constant_table bool_names[];
>  
>  #ifdef CONFIG_VALIDATE_FS_PARSER
> -extern bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
> -				    int low, int high, int special);
>  extern bool fs_validate_description(const char *name,
>  				    const struct fs_parameter_spec *desc);
>  #else
> -static inline bool validate_constant_table(const struct constant_table *tbl, size_t tbl_size,
> -					   int low, int high, int special)
> -{ return true; }
>  static inline bool fs_validate_description(const char *name,
>  					   const struct fs_parameter_spec *desc)
>  { return true; }
> 
> ---
> base-commit: 8cc42084abd926e3f005d7f5c23694c598b29cee
> change-id: 20250410-fix_fs-6e0a97c4e59f
> 
> Best regards,
> -- 
> Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR