Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415530 > unrolled thread

[PATCH] Fix NFS option parsing bit-rot.

Back to article view | Back to linux.kernel

Contents

  [PATCH] Fix NFS option parsing bit-rot. Rob Landley <rob@landley.net> - 2016-06-07 01:00 +0200
    Re: [PATCH] Fix NFS option parsing bit-rot. Trond Myklebust <trondmy@primarydata.com> - 2016-06-07 02:00 +0200
      Re: [PATCH] Fix NFS option parsing bit-rot. Rob Landley <rob@landley.net> - 2016-06-08 21:50 +0200
        Re: [PATCH] Fix NFS option parsing bit-rot. Trond Myklebust <trondmy@primarydata.com> - 2016-06-08 22:40 +0200
          Re: [PATCH] Fix NFS option parsing bit-rot. Rob Landley <rob@landley.net> - 2016-06-09 00:00 +0200

#1415530 — [PATCH] Fix NFS option parsing bit-rot.

From: Rob Landley <rob@landley.net>

The kernel has string parsing code for NFS mount options, but it seems
to have bit-rotted over the years, so toybox mount needs the following
patch to be able to mount nfs. Without it, the kernel returns "invalid
argument" before sending any network traffic.

For more information, see
http://lists.landley.net/pipermail/toybox-landley.net/2016-March/004790.html

Signed-off-by: Rob Landley <rob@landley.net>
---

 fs/nfs/super.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2137e02..863585d 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2130,11 +2130,23 @@ static int nfs_validate_text_mount_data(void *options,
 	int port = 0;
 	int max_namelen = PAGE_SIZE;
 	int max_pathlen = NFS_MAXPATHLEN;
+	int rc;
 	struct sockaddr *sap = (struct sockaddr *)&args->nfs_server.address;
 
 	if (nfs_parse_mount_options((char *)options, args) == 0)
 		return -EINVAL;
 
+	rc = nfs_parse_devname(dev_name,
+				   &args->nfs_server.hostname,
+				   max_namelen,
+				   &args->nfs_server.export_path,
+				   max_pathlen);
+
+	args->nfs_server.addrlen = rpc_pton(args->net,
+			args->nfs_server.hostname,
+			strlen(args->nfs_server.hostname),
+			sap, sizeof(args->nfs_server.address));
+
 	if (!nfs_verify_server_address(sap))
 		goto out_no_address;
 
@@ -2155,11 +2167,7 @@ static int nfs_validate_text_mount_data(void *options,
 
 	nfs_set_port(sap, &args->nfs_server.port, port);
 
-	return nfs_parse_devname(dev_name,
-				   &args->nfs_server.hostname,
-				   max_namelen,
-				   &args->nfs_server.export_path,
-				   max_pathlen);
+	return rc;
 
 #if !IS_ENABLED(CONFIG_NFS_V4)
 out_v4_not_compiled:

[toc] | [next] | [standalone]

#1415586

On 6/6/16, 18:58, "Rob Landley" <rob@landley.net> wrote:

>From: Rob Landley <rob@landley.net>
>
>The kernel has string parsing code for NFS mount options, but it seems
>to have bit-rotted over the years, so toybox mount needs the following
>patch to be able to mount nfs. Without it, the kernel returns "invalid
>argument" before sending any network traffic.
>
>For more information, see
>http://lists.landley.net/pipermail/toybox-landley.net/2016-March/004790.html
>
>Signed-off-by: Rob Landley <rob@landley.net>
>---
>
> fs/nfs/super.c |   18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
>diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>index 2137e02..863585d 100644
>--- a/fs/nfs/super.c
>+++ b/fs/nfs/super.c
>@@ -2130,11 +2130,23 @@ static int nfs_validate_text_mount_data(void *options,
> 	int port = 0;
> 	int max_namelen = PAGE_SIZE;
> 	int max_pathlen = NFS_MAXPATHLEN;
>+	int rc;
> 	struct sockaddr *sap = (struct sockaddr *)&args->nfs_server.address;
> 
> 	if (nfs_parse_mount_options((char *)options, args) == 0)
> 		return -EINVAL;
> 
>+	rc = nfs_parse_devname(dev_name,
>+				   &args->nfs_server.hostname,
>+				   max_namelen,
>+				   &args->nfs_server.export_path,
>+				   max_pathlen);
>+
>+	args->nfs_server.addrlen = rpc_pton(args->net,
>+			args->nfs_server.hostname,
>+			strlen(args->nfs_server.hostname),
>+			sap, sizeof(args->nfs_server.address));
>+

That will scribble over the parsed address.

Trond

[toc] | [prev] | [next] | [standalone]

#1417811

On 06/06/2016 06:49 PM, Trond Myklebust wrote:
> On 6/6/16, 18:58, "Rob Landley" <rob@landley.net> wrote:
> 
>>From: Rob Landley <rob@landley.net>
>>
>>The kernel has string parsing code for NFS mount options, but it seems
>>to have bit-rotted over the years, so toybox mount needs the following
>>patch to be able to mount nfs. Without it, the kernel returns "invalid
>>argument" before sending any network traffic.
>>
>>For more information, see
>>http://lists.landley.net/pipermail/toybox-landley.net/2016-March/004790.html
...
> That will scribble over the parsed address.

You mean if you supply both -o addr=host and host:/ path name?
Because you can't leave off host:/ or it errors. If you do:

  mount("/tmp","/mnt","nfs",1,
    "port=9999,mountport=9999,nolock,v3,udp,addr=10.0.2.2");

It barfs because nfs_parse_devname() does:

  end = strchr(dev_name, ':');
  if (end == NULL)
    goto out_bad_devname;
...
out_bad_devname:
  dfprintk(MOUNT, "NFS: device name not in host:path format\n");
  return -EINVAL;

So addr= is at _best_ redundant. You MUST supply host:/ always,
the current code just wasn't using it. My patch makes it use it.

There's no other way to get the address set for the text
parsing path, when we enter nfs_validate_text_mount_data()
(the function I patched), mount_info.parsed will always be
zero.

But sure, he's a gratuitous for loop checking that you haven't
redundantly supplied both -o addr=host and host://path

Signed-off-by: Rob Landley <rob@landley.net>

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2137e02..9b62d8b 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2130,11 +2130,28 @@ static int nfs_validate_text_mount_data(void *options,
 	int port = 0;
 	int max_namelen = PAGE_SIZE;
 	int max_pathlen = NFS_MAXPATHLEN;
+	int rc;
+	int i;
+	char *c;
 	struct sockaddr *sap = (struct sockaddr *)&args->nfs_server.address;
 
 	if (nfs_parse_mount_options((char *)options, args) == 0)
 		return -EINVAL;
 
+	rc = nfs_parse_devname(dev_name,
+				   &args->nfs_server.hostname,
+				   max_namelen,
+				   &args->nfs_server.export_path,
+				   max_pathlen);
+
+	for (i = 0, c = (void *)sap; i<sizeof(*sap); i++)
+		if (c[i]) break;
+	if (i == sizeof(*sap))
+		args->nfs_server.addrlen = rpc_pton(args->net,
+				args->nfs_server.hostname,
+				strlen(args->nfs_server.hostname),
+				sap, sizeof(args->nfs_server.address));
+
 	if (!nfs_verify_server_address(sap))
 		goto out_no_address;
 
@@ -2155,11 +2172,7 @@ static int nfs_validate_text_mount_data(void *options,
 
 	nfs_set_port(sap, &args->nfs_server.port, port);
 
-	return nfs_parse_devname(dev_name,
-				   &args->nfs_server.hostname,
-				   max_namelen,
-				   &args->nfs_server.export_path,
-				   max_pathlen);
+	return rc;
 
 #if !IS_ENABLED(CONFIG_NFS_V4)
 out_v4_not_compiled:

[toc] | [prev] | [next] | [standalone]

#1417855

On 6/8/16, 15:41, "Rob Landley" <rob@landley.net> wrote:
>So addr= is at _best_ redundant. You MUST supply host:/ always,
>the current code just wasn't using it.

That’s because you just happen to be supplying an IP address instead of a hostname. The kernel has no DNS resolving functionality. It cannot resolve hostnames into IP addresses without help either by the caller or by means of an upcall. That is why ‘addr=’ has been a mandatory parameter ever since we introduced the text based parser.

Now we could, theoretically, have the client call nfs_dns_resolve_name() on the hostname to resolve it. However that breaks when you have net namespaces and such, since the kernel’s dns_query() call is not container aware.

Trond

[toc] | [prev] | [next] | [standalone]

#1417916

On 06/08/2016 03:35 PM, Trond Myklebust wrote:
> 
> 
> On 6/8/16, 15:41, "Rob Landley" <rob@landley.net> wrote:
>>So addr= is at _best_ redundant. You MUST supply host:/ always,
>>the current code just wasn't using it.
> 
> That’s because you just happen to be supplying an IP address instead of
> a hostname. The kernel has no DNS resolving functionality. It cannot
> resolve hostnames into IP addresses without help either by the caller or
> by means of an upcall. That is why ‘addr=’ has been a mandatory
> parameter ever since we introduced the text based parser.

Then why is host: mandatory in the dev name?

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web