diff mbox series

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

Message ID 20250415-fix_fs-v4-1-5d575124a3ff@quicinc.com (mailing list archive)
State New
Headers show
Series [v4] fs/fs_parse: Remove unused and problematic validate_constant_table() | expand

Commit Message

Zijun Hu April 15, 2025, 12:25 p.m. UTC 
From: Zijun Hu <quic_zijuhu@quicinc.com>

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@quicinc.com>
---
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@quicinc.com

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@quicinc.com

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@quicinc.com
---
 Documentation/filesystems/mount_api.rst | 15 ----------
 fs/fs_parser.c                          | 49 ---------------------------------
 include/linux/fs_parser.h               |  5 ----
 3 files changed, 69 deletions(-)


---
base-commit: 8cc42084abd926e3f005d7f5c23694c598b29cee
change-id: 20250410-fix_fs-6e0a97c4e59f

Best regards,

Comments

Jan Kara April 15, 2025, 4:39 p.m. UTC | #1 
On Tue 15-04-25 20:25:00, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> 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@quicinc.com>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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@quicinc.com
> 
> 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@quicinc.com
> 
> 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@quicinc.com
> ---
>  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@quicinc.com>
>
Christian Brauner April 16, 2025, 1:57 p.m. UTC | #2 
On Tue, 15 Apr 2025 20:25:00 +0800, Zijun Hu wrote:
> 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), ...)
> 
> [...]

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/1] fs/fs_parse: Remove unused and problematic validate_constant_table()
      https://git.kernel.org/vfs/vfs/c/02c827b74082
diff mbox series

Patch

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; }