[PATCH 0/4] enhance the path resolution capability in fs_parser

[PATCH 0/4] enhance the path resolution capability in fs_parser

[Hongbo Li]

Mount options with path should be parsed into block device or inode. As
the new mount API provides a serial of parsers, path should also be 
looked up into block device within these parsers, not in each specific
filesystem.

The following is a brief overview of the patches, see the patches for
more details.

Patch 1-2: Enhance the path resolution capability in fs_parser.
Patch 3: Fix the `journal_path` options error in ext4. 
Patch 4: Remove the `fs_lookup_param` and its description.

Comments and questions are, as always, welcome.

Thanks,
Hongbo

Hongbo Li (4):
  fs: add blockdev parser for filesystem mount option.
  fs: add path parser for filesystem mount option.
  fs: ext4: support relative path for `journal_path` in mount option.
  fs: remove fs_lookup_param and its description.

 Documentation/filesystems/mount_api.rst |  17 +---
 fs/ext4/super.c                         |  26 +----
 fs/fs_parser.c                          | 125 +++++++++++++-----------
 include/linux/fs_parser.h               |   7 +-
 4 files changed, 71 insertions(+), 104 deletions(-)

-- 
2.34.1

[PATCH 1/4] fs: add blockdev parser for filesystem mount option.

[Hongbo Li]

`fsparam_bdev` uses `fs_param_is_blockdev` to parse the option, but it
is currently empty. Filesystems like ext4 use the `fsparam_bdev` to
parse `journal_path` into the block device. This general logic should
be moved to the vfs layer, not the specific filesystem. Therefore, we
implement block device parser in `fs_param_is_blockdev`. And the logic
is similar with `fs_lookup_param`.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/fs_parser.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index a4d6ca0b8971..48f60ecfcca0 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -311,7 +311,56 @@ EXPORT_SYMBOL(fs_param_is_fd);
 int fs_param_is_blockdev(struct p_log *log, const struct fs_parameter_spec *p,
 		  struct fs_parameter *param, struct fs_parse_result *result)
 {
-	return 0;
+	int ret;
+	int dfd;
+	struct filename *f;
+	struct inode *dev_inode;
+	struct path path;
+	bool put_f;
+
+	switch (param->type) {
+	case fs_value_is_string:
+		if (!*param->string) {
+			if (p->flags & fs_param_can_be_empty)
+				return 0;
+			break;
+		}
+		f = getname_kernel(param->string);
+		if (IS_ERR(f))
+			return fs_param_bad_value(log, param);
+		dfd = AT_FDCWD;
+		put_f = true;
+		break;
+	case fs_value_is_filename:
+		f = param->name;
+		dfd = param->dirfd;
+		put_f = false;
+		break;
+	default:
+		return fs_param_bad_value(log, param);
+	}
+
+	ret = filename_lookup(dfd, f, LOOKUP_FOLLOW, &path, NULL);
+	if (ret < 0) {
+		error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
+		goto out_putname;
+	}
+
+	dev_inode = d_backing_inode(path.dentry);
+	if (!S_ISBLK(dev_inode->i_mode)) {
+		error_plog(log, "%s: Non-blockdev passed as '%s'", param->key, f->name);
+		ret = -1;
+		goto out_putpath;
+	}
+	result->uint_32 = new_encode_dev(dev_inode->i_rdev);
+
+out_putpath:
+	path_put(&path);
+out_putname:
+	if (put_f)
+		putname(f);
+
+	return (ret < 0) ? fs_param_bad_value(log, param) : 0;
 }
 EXPORT_SYMBOL(fs_param_is_blockdev);
 
-- 
2.34.1

[PATCH 2/4] fs: add path parser for filesystem mount option.

[Hongbo Li]

`fsparam_path` uses `fs_param_is_path` to parse the option, but it
is currently empty. The new mount api has considered this option in
`fsconfig`(that is FSCONFIG_SET_PATH). Here we add general path parser
in filesystem layer. Currently, no filesystem uses this function to
parse parameters, we add `void *ptr` in `fs_parse_result` to point to
the target structure(such as `struct inode *`).

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/fs_parser.c            | 18 ++++++++++++++++++
 include/linux/fs_parser.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 48f60ecfcca0..c41a13e564c4 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -367,6 +367,24 @@ EXPORT_SYMBOL(fs_param_is_blockdev);
 int fs_param_is_path(struct p_log *log, const struct fs_parameter_spec *p,
 		     struct fs_parameter *param, struct fs_parse_result *result)
 {
+	int ret;
+	struct filename *f;
+	struct path path;
+
+	if (param->type != fs_value_is_filename)
+		return fs_param_bad_value(log, param);
+	if (!*param->string && (p->flags & fs_param_can_be_empty))
+		return 0;
+
+	f = param->name;
+	ret = filename_lookup(param->dirfd, f, LOOKUP_FOLLOW, &path, NULL);
+	if (ret < 0) {
+		error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
+		return fs_param_bad_value(log, param);
+	}
+	result->ptr = d_backing_inode(path.dentry);
+	path_put(&path);
+
 	return 0;
 }
 EXPORT_SYMBOL(fs_param_is_path);
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index d3350979115f..489c71d06a5f 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -57,6 +57,7 @@ struct fs_parse_result {
 		int		int_32;		/* For spec_s32/spec_enum */
 		unsigned int	uint_32;	/* For spec_u32{,_octal,_hex}/spec_enum */
 		u64		uint_64;	/* For spec_u64 */
+		const void	*ptr;		/* For spec_ptr */
 	};
 };
 
-- 
2.34.1

[PATCH 3/4] fs: ext4: support relative path for `journal_path` in mount option.

[Hongbo Li]

After `fs_param_is_blockdev` is implemented(in fs: add blockdev parser
for filesystem mount option.), `journal_devnum` can be obtained from
`result.uint_32` directly.

Additionally, the `fs_lookup_param` did not consider the relative path
for block device. When we mount ext4 with `journal_path` option using
relative path, `param->dirfd` was not set which will cause mounting
error.

This can be reproduced easily like this:

mke2fs -F -O journal_dev $JOURNAL_DEV -b 4096 100M
mkfs.ext4 -F -J device=$JOURNAL_DEV -b 4096 $FS_DEV
cd /dev; mount -t ext4 -o journal_path=`basename $JOURNAL_DEV` $FS_DEV $MNT

Fixes: 461c3af045d3 ("ext4: Change handle_mount_opt() to use fs_parameter")
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/ext4/super.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c682fb927b64..94b39bcae99d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2290,39 +2290,15 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		ctx->spec |= EXT4_SPEC_s_resgid;
 		return 0;
 	case Opt_journal_dev:
-		if (is_remount) {
-			ext4_msg(NULL, KERN_ERR,
-				 "Cannot specify journal on remount");
-			return -EINVAL;
-		}
-		ctx->journal_devnum = result.uint_32;
-		ctx->spec |= EXT4_SPEC_JOURNAL_DEV;
-		return 0;
 	case Opt_journal_path:
-	{
-		struct inode *journal_inode;
-		struct path path;
-		int error;
-
 		if (is_remount) {
 			ext4_msg(NULL, KERN_ERR,
 				 "Cannot specify journal on remount");
 			return -EINVAL;
 		}
-
-		error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path);
-		if (error) {
-			ext4_msg(NULL, KERN_ERR, "error: could not find "
-				 "journal device path");
-			return -EINVAL;
-		}
-
-		journal_inode = d_inode(path.dentry);
-		ctx->journal_devnum = new_encode_dev(journal_inode->i_rdev);
+		ctx->journal_devnum = result.uint_32;
 		ctx->spec |= EXT4_SPEC_JOURNAL_DEV;
-		path_put(&path);
 		return 0;
-	}
 	case Opt_journal_ioprio:
 		if (result.uint_32 > 7) {
 			ext4_msg(NULL, KERN_ERR, "Invalid journal IO priority"
-- 
2.34.1

[PATCH 4/4] fs: remove fs_lookup_param and its description.

[Hongbo Li]

After `fs_param_is_blockdev` and `fs_param_is_path` are implemented,
there are no need to lookup the path with `fs_lookup_param`, and
`fs_parse` is sufficient.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 Documentation/filesystems/mount_api.rst | 17 +-------
 fs/fs_parser.c                          | 56 -------------------------
 include/linux/fs_parser.h               |  6 ---
 3 files changed, 1 insertion(+), 78 deletions(-)

diff --git a/Documentation/filesystems/mount_api.rst b/Documentation/filesystems/mount_api.rst
index 9aaf6ef75eb5..f92d96758e57 100644
--- a/Documentation/filesystems/mount_api.rst
+++ b/Documentation/filesystems/mount_api.rst
@@ -253,7 +253,7 @@ manage the filesystem context.  They are as follows:
      will have been weeded out and fc->sb_flags updated in the context.
      Security options will also have been weeded out and fc->security updated.
 
-     The parameter can be parsed with fs_parse() and fs_lookup_param().  Note
+     The parameter can be parsed with fs_parse().  Note
      that the source(s) are presented as parameters named "source".
 
      If successful, 0 should be returned or a negative error code otherwise.
@@ -796,18 +796,3 @@ process the parameters it is given.
      If the parameter isn't matched, -ENOPARAM will be returned; if the
      parameter is matched, but the value is erroneous, -EINVAL will be
      returned; otherwise the parameter's option number will be returned.
-
-   * ::
-
-       int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *value,
-			   bool want_bdev,
-			   unsigned int flags,
-			   struct path *_path);
-
-     This takes a parameter that carries a string or filename type and attempts
-     to do a path lookup on it.  If the parameter expects a blockdev, a check
-     is made that the inode actually represents one.
-
-     Returns 0 if successful and ``*_path`` will be set; returns a negative
-     error code if not.
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index c41a13e564c4..071c0f9d0121 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -133,62 +133,6 @@ int __fs_parse(struct p_log *log,
 }
 EXPORT_SYMBOL(__fs_parse);
 
-/**
- * fs_lookup_param - Look up a path referred to by a parameter
- * @fc: The filesystem context to log errors through.
- * @param: The parameter.
- * @want_bdev: T if want a blockdev
- * @flags: Pathwalk flags passed to filename_lookup()
- * @_path: The result of the lookup
- */
-int fs_lookup_param(struct fs_context *fc,
-		    struct fs_parameter *param,
-		    bool want_bdev,
-		    unsigned int flags,
-		    struct path *_path)
-{
-	struct filename *f;
-	bool put_f;
-	int ret;
-
-	switch (param->type) {
-	case fs_value_is_string:
-		f = getname_kernel(param->string);
-		if (IS_ERR(f))
-			return PTR_ERR(f);
-		put_f = true;
-		break;
-	case fs_value_is_filename:
-		f = param->name;
-		put_f = false;
-		break;
-	default:
-		return invalf(fc, "%s: not usable as path", param->key);
-	}
-
-	ret = filename_lookup(param->dirfd, f, flags, _path, NULL);
-	if (ret < 0) {
-		errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name);
-		goto out;
-	}
-
-	if (want_bdev &&
-	    !S_ISBLK(d_backing_inode(_path->dentry)->i_mode)) {
-		path_put(_path);
-		_path->dentry = NULL;
-		_path->mnt = NULL;
-		errorf(fc, "%s: Non-blockdev passed as '%s'",
-		       param->key, f->name);
-		ret = -ENOTBLK;
-	}
-
-out:
-	if (put_f)
-		putname(f);
-	return ret;
-}
-EXPORT_SYMBOL(fs_lookup_param);
-
 static int fs_param_bad_value(struct p_log *log, struct fs_parameter *param)
 {
 	return inval_plog(log, "Bad value for '%s'", param->key);
diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
index 489c71d06a5f..fa2745254818 100644
--- a/include/linux/fs_parser.h
+++ b/include/linux/fs_parser.h
@@ -74,12 +74,6 @@ static inline int fs_parse(struct fs_context *fc,
 	return __fs_parse(&fc->log, desc, param, result);
 }
 
-extern int fs_lookup_param(struct fs_context *fc,
-			   struct fs_parameter *param,
-			   bool want_bdev,
-			   unsigned int flags,
-			   struct path *_path);
-
 extern int lookup_constant(const struct constant_table tbl[], const char *name, int not_found);
 
 #ifdef CONFIG_VALIDATE_FS_PARSER
-- 
2.34.1

Re: [PATCH 1/4] fs: add blockdev parser for filesystem mount option.

[kernel test robot]

Hi Hongbo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on brauner-vfs/vfs.all linus/master v6.10-rc1 next-20240523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hongbo-Li/fs-add-blockdev-parser-for-filesystem-mount-option/20240527-094930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20240527014717.690140-2-lihongbo22%40huawei.com
patch subject: [PATCH 1/4] fs: add blockdev parser for filesystem mount option.
config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20240527/202405271100.aGNNnrKK-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240527/202405271100.aGNNnrKK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405271100.aGNNnrKK-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/fs_parser.c:324:8: warning: variable 'f' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fs_parser.c:343:29: note: uninitialized use occurs here
     343 |         ret = filename_lookup(dfd, f, LOOKUP_FOLLOW, &path, NULL);
         |                                    ^
   fs/fs_parser.c:324:4: note: remove the 'if' if its condition is always true
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     325 |                                 return 0;
   fs/fs_parser.c:316:20: note: initialize the variable 'f' to silence this warning
     316 |         struct filename *f;
         |                           ^
         |                            = NULL
>> fs/fs_parser.c:324:8: warning: variable 'put_f' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fs_parser.c:360:6: note: uninitialized use occurs here
     360 |         if (put_f)
         |             ^~~~~
   fs/fs_parser.c:324:4: note: remove the 'if' if its condition is always true
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     325 |                                 return 0;
   fs/fs_parser.c:319:12: note: initialize the variable 'put_f' to silence this warning
     319 |         bool put_f;
         |                   ^
         |                    = 0
>> fs/fs_parser.c:324:8: warning: variable 'dfd' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/fs_parser.c:343:24: note: uninitialized use occurs here
     343 |         ret = filename_lookup(dfd, f, LOOKUP_FOLLOW, &path, NULL);
         |                               ^~~
   fs/fs_parser.c:324:4: note: remove the 'if' if its condition is always true
     324 |                         if (p->flags & fs_param_can_be_empty)
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     325 |                                 return 0;
   fs/fs_parser.c:315:9: note: initialize the variable 'dfd' to silence this warning
     315 |         int dfd;
         |                ^
         |                 = 0
   3 warnings generated.


vim +324 fs/fs_parser.c

   310	
   311	int fs_param_is_blockdev(struct p_log *log, const struct fs_parameter_spec *p,
   312			  struct fs_parameter *param, struct fs_parse_result *result)
   313	{
   314		int ret;
   315		int dfd;
   316		struct filename *f;
   317		struct inode *dev_inode;
   318		struct path path;
   319		bool put_f;
   320	
   321		switch (param->type) {
   322		case fs_value_is_string:
   323			if (!*param->string) {
 > 324				if (p->flags & fs_param_can_be_empty)
   325					return 0;
   326				break;
   327			}
   328			f = getname_kernel(param->string);
   329			if (IS_ERR(f))
   330				return fs_param_bad_value(log, param);
   331			dfd = AT_FDCWD;
   332			put_f = true;
   333			break;
   334		case fs_value_is_filename:
   335			f = param->name;
   336			dfd = param->dirfd;
   337			put_f = false;
   338			break;
   339		default:
   340			return fs_param_bad_value(log, param);
   341		}
   342	
   343		ret = filename_lookup(dfd, f, LOOKUP_FOLLOW, &path, NULL);
   344		if (ret < 0) {
   345			error_plog(log, "%s: Lookup failure for '%s'", param->key, f->name);
   346			goto out_putname;
   347		}
   348	
   349		dev_inode = d_backing_inode(path.dentry);
   350		if (!S_ISBLK(dev_inode->i_mode)) {
   351			error_plog(log, "%s: Non-blockdev passed as '%s'", param->key, f->name);
   352			ret = -1;
   353			goto out_putpath;
   354		}
   355		result->uint_32 = new_encode_dev(dev_inode->i_rdev);
   356	
   357	out_putpath:
   358		path_put(&path);
   359	out_putname:
   360		if (put_f)
   361			putname(f);
   362	
   363		return (ret < 0) ? fs_param_bad_value(log, param) : 0;
   364	}
   365	EXPORT_SYMBOL(fs_param_is_blockdev);
   366	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki