[PATCH] fs: avoid empty option when generating legacy mount string

[PATCH] fs: avoid empty option when generating legacy mount string

[Thomas Weißschuh]

As each option string fragment is always prepended with a comma it would
happen that the whole string always starts with a comma.
This could be interpreted by filesystem drivers as an empty option and
may produce errors.

For example the NTFS driver from ntfs.ko behaves like this and fails when
mounted via the new API.

Link: https://github.com/util-linux/util-linux/issues/2298
Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 fs/fs_context.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 24ce12f0db32..851214d1d013 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -561,7 +561,8 @@ static int legacy_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			return -ENOMEM;
 	}
 
-	ctx->legacy_data[size++] = ',';
+	if (size)
+		ctx->legacy_data[size++] = ',';
 	len = strlen(param->key);
 	memcpy(ctx->legacy_data + size, param->key, len);
 	size += len;

---
base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7
change-id: 20230607-fs-empty-option-265622371023

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

Re: [PATCH] fs: avoid empty option when generating legacy mount string

[Christian Brauner]

On Wed, Jun 07, 2023 at 07:28:48PM +0200, Thomas Weißschuh wrote:
> As each option string fragment is always prepended with a comma it would
> happen that the whole string always starts with a comma.
> This could be interpreted by filesystem drivers as an empty option and
> may produce errors.
> 
> For example the NTFS driver from ntfs.ko behaves like this and fails when
> mounted via the new API.
> 
> Link: https://github.com/util-linux/util-linux/issues/2298

Yeah, the old ntfs driver implements its own option parser. It
overwrites/splits at ',' returning '\0' and then trips over this.

Contrast with e.g., ovl_next_op() which does the same thing but skips
over '\0' in ovl_parse_opt().

So arguably also a bug in ntfs parsing. But there's no reason we should
prepend ',' for legacy mount option strings.

And yeah, I can easily verify this...

Using my custom move-mount tool I originally wrote for another patchset
but which is handy to pass mount options via the new mount api _system_
calls and not via mount():
https://github.com/brauner/move-mount-beneath

I can do:

        sudo ./move-mount -f overlay -olowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged

and clearly see:

        > sudo bpftrace -e 'kfunc:legacy_get_tree { @m = args->fc; printf("%s\n", str(((struct legacy_fs_context *)@m->fs_private)->legacy_data)); }'
        Attaching 1 probe...
        ,lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work

> Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")

Should be:

Fixes: commit 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")

and misses a:

Cc: stable@vger.kernel.org

I'll fix this up for you though.

Re: [PATCH] fs: avoid empty option when generating legacy mount string

[Thomas Weißschuh]

On 2023-06-07 21:39:01+0200, Christian Brauner wrote:
> On Wed, Jun 07, 2023 at 07:28:48PM +0200, Thomas Weißschuh wrote:
> > As each option string fragment is always prepended with a comma it would
> > happen that the whole string always starts with a comma.
> > This could be interpreted by filesystem drivers as an empty option and
> > may produce errors.
> > 
> > For example the NTFS driver from ntfs.ko behaves like this and fails when
> > mounted via the new API.
> > 
> > Link: https://github.com/util-linux/util-linux/issues/2298
> 
> Yeah, the old ntfs driver implements its own option parser. It
> overwrites/splits at ',' returning '\0' and then trips over this.
> 
> Contrast with e.g., ovl_next_op() which does the same thing but skips
> over '\0' in ovl_parse_opt().
> 
> So arguably also a bug in ntfs parsing. But there's no reason we should
> prepend ',' for legacy mount option strings.
> 
> And yeah, I can easily verify this...
> 
> Using my custom move-mount tool I originally wrote for another patchset
> but which is handy to pass mount options via the new mount api _system_
> calls and not via mount():
> https://github.com/brauner/move-mount-beneath
> 
> I can do:
> 
>         sudo ./move-mount -f overlay -olowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged
> 
> and clearly see:
> 
>         > sudo bpftrace -e 'kfunc:legacy_get_tree { @m = args->fc; printf("%s\n", str(((struct legacy_fs_context *)@m->fs_private)->legacy_data)); }'
>         Attaching 1 probe...
>         ,lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work
> 
> > Fixes: 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")
> 
> Should be:
> 
> Fixes: commit 3e1aeb00e6d1 ("vfs: Implement a filesystem superblock creation/configuration context")

AFAIK the Fixes: tag does not use the "commit" keyword. Only inline
commit references.

This is how it's currently documented in
Documentation/process/submitting-patches.rst.

> and misses a:
> 
> Cc: stable@vger.kernel.org

This was fixed in v2.

> I'll fix this up for you though.

Thanks!


Thomas

Re: [PATCH] fs: avoid empty option when generating legacy mount string

[Christian Brauner]

On Wed, 07 Jun 2023 19:28:48 +0200, Thomas Weißschuh wrote:
> As each option string fragment is always prepended with a comma it would
> happen that the whole string always starts with a comma.
> This could be interpreted by filesystem drivers as an empty option and
> may produce errors.
> 
> For example the NTFS driver from ntfs.ko behaves like this and fails when
> mounted via the new API.
> 
> [...]

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

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

[1/1] fs: avoid empty option when generating legacy mount string
      https://git.kernel.org/vfs/vfs/c/de3824801c82