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

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

[Hongbo Li]

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")
Suggested-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
v2:
  - Change the journal_path parameter as string not bdev, and
    determine the relative path situation inside fs_lookup_param.
  - Add Suggested-by.

v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/
---
 fs/ext4/super.c | 4 ++--
 fs/fs_parser.c  | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 16a4ce704460..cd23536ce46e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
 	fsparam_u32	("min_batch_time",	Opt_min_batch_time),
 	fsparam_u32	("max_batch_time",	Opt_max_batch_time),
 	fsparam_u32	("journal_dev",		Opt_journal_dev),
-	fsparam_bdev	("journal_path",	Opt_journal_path),
+	fsparam_string	("journal_path",	Opt_journal_path),
 	fsparam_flag	("journal_checksum",	Opt_journal_checksum),
 	fsparam_flag	("nojournal_checksum",	Opt_nojournal_checksum),
 	fsparam_flag	("journal_async_commit",Opt_journal_async_commit),
@@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			return -EINVAL;
 		}
 
-		error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path);
+		error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path);
 		if (error) {
 			ext4_msg(NULL, KERN_ERR, "error: could not find "
 				 "journal device path");
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 24727ec34e5a..2ae296764b69 100644
--- a/fs/fs_parser.c
+++ b/fs/fs_parser.c
@@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
 		f = getname_kernel(param->string);
 		if (IS_ERR(f))
 			return PTR_ERR(f);
+		/* for relative path */
+		if (f->name[0] != '/')
+			param->dirfd = AT_FDCWD;
 		put_f = true;
 		break;
 	case fs_value_is_filename:
-- 
2.34.1

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Al Viro]

On Wed, Sep 25, 2024 at 09:56:24AM +0800, Hongbo Li wrote:
> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
>  		f = getname_kernel(param->string);
>  		if (IS_ERR(f))
>  			return PTR_ERR(f);
> +		/* for relative path */
> +		if (f->name[0] != '/')
> +			param->dirfd = AT_FDCWD;

Will need to dig around for some context, but this bit definitely makes
no sense - dirfd is completely ignored for absolute pathnames, so making
that store conditional is pointless.

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Hongbo Li]

On 2024/9/25 10:09, Al Viro wrote:
> On Wed, Sep 25, 2024 at 09:56:24AM +0800, Hongbo Li wrote:
>> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
>>   		f = getname_kernel(param->string);
>>   		if (IS_ERR(f))
>>   			return PTR_ERR(f);
>> +		/* for relative path */
>> +		if (f->name[0] != '/')
>> +			param->dirfd = AT_FDCWD;
> 
> Will need to dig around for some context, but this bit definitely makes
> no sense - dirfd is completely ignored for absolute pathnames, so making
> that store conditional is pointless.
> 

Only do it for relative path. As mentioned in [1], if the "journal_path" 
is treated as FSCONFIG_SET_PATH may be better, but mount(8) is passing a 
string (which uses FSCONFIG_SET_STRING for "journal_path"). For the 
relative path case, the dirfd should be assigned.

[1] 
https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/

Thanks,
Hongbo

> 

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Jan Kara]

On Wed 25-09-24 09:56:24, Hongbo Li wrote:
> 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")
> Suggested-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
> v2:
>   - Change the journal_path parameter as string not bdev, and
>     determine the relative path situation inside fs_lookup_param.
>   - Add Suggested-by.
> 
> v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/
> ---
>  fs/ext4/super.c | 4 ++--
>  fs/fs_parser.c  | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 16a4ce704460..cd23536ce46e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
>  	fsparam_u32	("min_batch_time",	Opt_min_batch_time),
>  	fsparam_u32	("max_batch_time",	Opt_max_batch_time),
>  	fsparam_u32	("journal_dev",		Opt_journal_dev),
> -	fsparam_bdev	("journal_path",	Opt_journal_path),
> +	fsparam_string	("journal_path",	Opt_journal_path),

Why did you change this? As far as I can see the only effect would be that
empty path will not be allowed (which makes sense) but that seems like an
independent change which would deserve a comment in the changelog? Or am I
missing something?

>  	fsparam_flag	("journal_checksum",	Opt_journal_checksum),
>  	fsparam_flag	("nojournal_checksum",	Opt_nojournal_checksum),
>  	fsparam_flag	("journal_async_commit",Opt_journal_async_commit),
> @@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  			return -EINVAL;
>  		}
>  
> -		error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path);
> +		error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path);
>  		if (error) {
>  			ext4_msg(NULL, KERN_ERR, "error: could not find "
>  				 "journal device path");
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index 24727ec34e5a..2ae296764b69 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
>  		f = getname_kernel(param->string);
>  		if (IS_ERR(f))
>  			return PTR_ERR(f);
> +		/* for relative path */
> +		if (f->name[0] != '/')
> +			param->dirfd = AT_FDCWD;

What Al meant is that you can do simply:
		param->dirfd = AT_FDCWD;

and everything will work the same because 'dfd' is ignored for absolute
pathnames in path_init().

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

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Hongbo Li]

On 2024/9/25 15:51, Jan Kara wrote:
> On Wed 25-09-24 09:56:24, Hongbo Li wrote:
>> 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")
>> Suggested-by: Christian Brauner <brauner@kernel.org>
>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
>> ---
>> v2:
>>    - Change the journal_path parameter as string not bdev, and
>>      determine the relative path situation inside fs_lookup_param.
>>    - Add Suggested-by.
>>
>> v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/
>> ---
>>   fs/ext4/super.c | 4 ++--
>>   fs/fs_parser.c  | 3 +++
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 16a4ce704460..cd23536ce46e 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
>>   	fsparam_u32	("min_batch_time",	Opt_min_batch_time),
>>   	fsparam_u32	("max_batch_time",	Opt_max_batch_time),
>>   	fsparam_u32	("journal_dev",		Opt_journal_dev),
>> -	fsparam_bdev	("journal_path",	Opt_journal_path),
>> +	fsparam_string	("journal_path",	Opt_journal_path),
> 
> Why did you change this? As far as I can see the only effect would be that
> empty path will not be allowed (which makes sense) but that seems like an
> independent change which would deserve a comment in the changelog? Or am I
> missing something?

The fsparam_bdev serves no purpose here, you're right, empty path will 
not be allowed for `journal_path` option. And all changes are made to 
fix the issues (journal_path options changed) introduced by the previous 
new mount api conversion.

> 
>>   	fsparam_flag	("journal_checksum",	Opt_journal_checksum),
>>   	fsparam_flag	("nojournal_checksum",	Opt_nojournal_checksum),
>>   	fsparam_flag	("journal_async_commit",Opt_journal_async_commit),
>> @@ -2301,7 +2301,7 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>   			return -EINVAL;
>>   		}
>>   
>> -		error = fs_lookup_param(fc, param, 1, LOOKUP_FOLLOW, &path);
>> +		error = fs_lookup_param(fc, param, true, LOOKUP_FOLLOW, &path);
>>   		if (error) {
>>   			ext4_msg(NULL, KERN_ERR, "error: could not find "
>>   				 "journal device path");
>> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
>> index 24727ec34e5a..2ae296764b69 100644
>> --- a/fs/fs_parser.c
>> +++ b/fs/fs_parser.c
>> @@ -156,6 +156,9 @@ int fs_lookup_param(struct fs_context *fc,
>>   		f = getname_kernel(param->string);
>>   		if (IS_ERR(f))
>>   			return PTR_ERR(f);
>> +		/* for relative path */
>> +		if (f->name[0] != '/')
>> +			param->dirfd = AT_FDCWD;
> 
> What Al meant is that you can do simply:
> 		param->dirfd = AT_FDCWD;
> 
> and everything will work the same because 'dfd' is ignored for absolute
> pathnames in path_init().
> 
ok, seems reasonable.

Thanks,
Hongbo
> 								Honza

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Christian Brauner]

On Wed, Sep 25, 2024 at 09:51:05AM GMT, Jan Kara wrote:
> On Wed 25-09-24 09:56:24, Hongbo Li wrote:
> > 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")
> > Suggested-by: Christian Brauner <brauner@kernel.org>
> > Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> > ---
> > v2:
> >   - Change the journal_path parameter as string not bdev, and
> >     determine the relative path situation inside fs_lookup_param.
> >   - Add Suggested-by.
> > 
> > v1: https://lore.kernel.org/all/20240527-mahlen-packung-3fe035ab390d@brauner/
> > ---
> >  fs/ext4/super.c | 4 ++--
> >  fs/fs_parser.c  | 3 +++
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 16a4ce704460..cd23536ce46e 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -1744,7 +1744,7 @@ static const struct fs_parameter_spec ext4_param_specs[] = {
> >  	fsparam_u32	("min_batch_time",	Opt_min_batch_time),
> >  	fsparam_u32	("max_batch_time",	Opt_max_batch_time),
> >  	fsparam_u32	("journal_dev",		Opt_journal_dev),
> > -	fsparam_bdev	("journal_path",	Opt_journal_path),
> > +	fsparam_string	("journal_path",	Opt_journal_path),
> 
> Why did you change this? As far as I can see the only effect would be that
> empty path will not be allowed (which makes sense) but that seems like an
> independent change which would deserve a comment in the changelog? Or am I
> missing something?

I'll drop the ext4 bit as that can be done independently drop the
conditional.

Re: [PATCH v2] fs: ext4: support relative path for `journal_path` in mount option.

[Christian Brauner]

On Wed, 25 Sep 2024 09:56:24 +0800, Hongbo Li wrote:
> 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:
> 
> [...]

Applied to the vfs.misc.v6.13 branch of the vfs/vfs.git tree.
Patches in the vfs.misc.v6.13 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.misc.v6.13

[1/1] fs: ext4: support relative path for `journal_path` in mount option.
      https://git.kernel.org/vfs/vfs/c/457f7b53e736