2024-01-30 15:47:14

by Chen Hanxiao

Subject: [PATCH v2] nfs: fix regression in handling of fsc= option in NFSv4

Setting the uniquifier for fscache via the fsc= mount
option is currently broken in NFSv4.

Fix this by passing fscache_uniq to root_fc if possible.

Signed-off-by: Chen Hanxiao <[email protected]>
---
v2:
use kmemdup_nul instead of snprintf

fs/nfs/nfs4super.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
index d09bcfd7db89..4a23d9143d5a 100644
--- a/fs/nfs/nfs4super.c
+++ b/fs/nfs/nfs4super.c
@@ -145,6 +145,7 @@ static int do_nfs4_mount(struct nfs_server *server,
const char *export_path)
{
struct nfs_fs_context *root_ctx;
+ struct nfs_fs_context *ctx;
struct fs_context *root_fc;
struct vfsmount *root_mnt;
struct dentry *dentry;
@@ -157,6 +158,12 @@ static int do_nfs4_mount(struct nfs_server *server,
.dirfd = -1,
};

+ struct fs_parameter param_fsc = {
+ .key = "fsc",
+ .type = fs_value_is_string,
+ .dirfd = -1,
+ };
+
if (IS_ERR(server))
return PTR_ERR(server);

@@ -168,9 +175,26 @@ static int do_nfs4_mount(struct nfs_server *server,
kfree(root_fc->source);
root_fc->source = NULL;

+ ctx = nfs_fc2context(fc);
root_ctx = nfs_fc2context(root_fc);
root_ctx->internal = true;
root_ctx->server = server;
+
+ if (ctx->fscache_uniq) {
+ len = strlen(ctx->fscache_uniq) + 1;
+ param_fsc.size = len;
+ param_fsc.string = kmemdup_nul(ctx->fscache_uniq, len, GFP_KERNEL);
+ if (param_fsc.string == NULL) {
+ put_fs_context(root_fc);
+ return -ENOMEM;
+ }
+ ret = vfs_parse_fs_param(root_fc, &param_fsc);
+ kfree(param_fsc.string);
+ if (ret < 0) {
+ put_fs_context(root_fc);
+ return ret;
+ }
+ }
/* We leave export_path unset as it's not used to find the root. */

len = strlen(hostname) + 5;
--
2.39.1

2024-01-30 15:50:41

by Jeff Layton

Subject: Re: [PATCH v2] nfs: fix regression in handling of fsc= option in NFSv4

On Tue, 2024-01-30 at 23:46 +0800, Chen Hanxiao wrote:
> Setting the uniquifier for fscache via the fsc= mount
> option is currently broken in NFSv4.
>
> Fix this by passing fscache_uniq to root_fc if possible.
>
> Signed-off-by: Chen Hanxiao <[email protected]>
> ---
> v2:
> use kmemdup_nul instead of snprintf
>
> fs/nfs/nfs4super.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> index d09bcfd7db89..4a23d9143d5a 100644
> --- a/fs/nfs/nfs4super.c
> +++ b/fs/nfs/nfs4super.c
> @@ -145,6 +145,7 @@ static int do_nfs4_mount(struct nfs_server *server,
> const char *export_path)
> {
> struct nfs_fs_context *root_ctx;
> + struct nfs_fs_context *ctx;
> struct fs_context *root_fc;
> struct vfsmount *root_mnt;
> struct dentry *dentry;
> @@ -157,6 +158,12 @@ static int do_nfs4_mount(struct nfs_server *server,
> .dirfd = -1,
> };
>
> + struct fs_parameter param_fsc = {
> + .key = "fsc",
> + .type = fs_value_is_string,
> + .dirfd = -1,
> + };
> +
> if (IS_ERR(server))
> return PTR_ERR(server);
>
> @@ -168,9 +175,26 @@ static int do_nfs4_mount(struct nfs_server *server,
> kfree(root_fc->source);
> root_fc->source = NULL;
>
> + ctx = nfs_fc2context(fc);
> root_ctx = nfs_fc2context(root_fc);
> root_ctx->internal = true;
> root_ctx->server = server;
> +
> + if (ctx->fscache_uniq) {
> + len = strlen(ctx->fscache_uniq) + 1;
> + param_fsc.size = len;

In the other patch you were setting .size to the return value from
snprintf. That does not include the NUL byte. You're including it here
though. Which way is correct?

> + param_fsc.string = kmemdup_nul(ctx->fscache_uniq, len, GFP_KERNEL);
> + if (param_fsc.string == NULL) {
> + put_fs_context(root_fc);
> + return -ENOMEM;
> + }
> + ret = vfs_parse_fs_param(root_fc, &param_fsc);
> + kfree(param_fsc.string);
> + if (ret < 0) {
> + put_fs_context(root_fc);
> + return ret;
> + }
> + }
> /* We leave export_path unset as it's not used to find the root. */
>
> len = strlen(hostname) + 5;

--
Jeff Layton <[email protected]>

2024-01-31 04:05:04

by Chen Hanxiao

Subject: Re: [PATCH v2] nfs: fix regression in handling of fsc= option in NFSv4

> -----?ʼ?ԭ??-----
> ??????: Jeff Layton <[email protected]>
> ????ʱ??: 2024??1??30?? 23:50
> ?ռ???: Chen, Hanxiao/?? ???? <[email protected]>; Trond Myklebust
> <[email protected]>; Anna Schumaker <[email protected]>
> ????: [email protected]; Dave Wysochanski <[email protected]>;
> David Howells <[email protected]>
> ????: Re: [PATCH v2] nfs: fix regression in handling of fsc= option in NFSv4
>
> On Tue, 2024-01-30 at 23:46 +0800, Chen Hanxiao wrote:
> > Setting the uniquifier for fscache via the fsc= mount
> > option is currently broken in NFSv4.
> >
> > Fix this by passing fscache_uniq to root_fc if possible.
> >
> > Signed-off-by: Chen Hanxiao <[email protected]>
> > ---
> > v2:
> > use kmemdup_nul instead of snprintf
> >
> > fs/nfs/nfs4super.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4super.c b/fs/nfs/nfs4super.c
> > index d09bcfd7db89..4a23d9143d5a 100644
> > --- a/fs/nfs/nfs4super.c
> > +++ b/fs/nfs/nfs4super.c
> > @@ -145,6 +145,7 @@ static int do_nfs4_mount(struct nfs_server *server,
> > const char *export_path)
> > {
> > struct nfs_fs_context *root_ctx;
> > + struct nfs_fs_context *ctx;
> > struct fs_context *root_fc;
> > struct vfsmount *root_mnt;
> > struct dentry *dentry;
> > @@ -157,6 +158,12 @@ static int do_nfs4_mount(struct nfs_server *server,
> > .dirfd = -1,
> > };
> >
> > + struct fs_parameter param_fsc = {
> > + .key = "fsc",
> > + .type = fs_value_is_string,
> > + .dirfd = -1,
> > + };
> > +
> > if (IS_ERR(server))
> > return PTR_ERR(server);
> >
> > @@ -168,9 +175,26 @@ static int do_nfs4_mount(struct nfs_server *server,
> > kfree(root_fc->source);
> > root_fc->source = NULL;
> >
> > + ctx = nfs_fc2context(fc);
> > root_ctx = nfs_fc2context(root_fc);
> > root_ctx->internal = true;
> > root_ctx->server = server;
> > +
> > + if (ctx->fscache_uniq) {
> > + len = strlen(ctx->fscache_uniq) + 1;
> > + param_fsc.size = len;
>
> In the other patch you were setting .size to the return value from
> snprintf. That does not include the NUL byte. You're including it here
> though. Which way is correct?
>
It should be:
len = strlen(ctx->fscache_uniq).
param_fsc.size = len;

for kmemdup_nul case.

Will be fixed in v3.
Sorry for the noise.

Regards
- Chen

> > + param_fsc.string = kmemdup_nul(ctx->fscache_uniq, len,
> GFP_KERNEL);
> > + if (param_fsc.string == NULL) {
> > + put_fs_context(root_fc);
> > + return -ENOMEM;
> > + }
> > + ret = vfs_parse_fs_param(root_fc, &param_fsc);
> > + kfree(param_fsc.string);
> > + if (ret < 0) {
> > + put_fs_context(root_fc);
> > + return ret;
> > + }
> > + }
> > /* We leave export_path unset as it's not used to find the root. */
> >
> > len = strlen(hostname) + 5;
>
> --
> Jeff Layton <[email protected]>