[PATCH] Fix NFS option parsing bit-rot.

[PATCH] Fix NFS option parsing bit-rot.

[Rob Landley]

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:

Re: [PATCH] Fix NFS option parsing bit-rot.

[Trond Myklebust]

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

Re: [PATCH] Fix NFS option parsing bit-rot.

[Trond Myklebust]

DQoNCk9uIDYvNi8xNiwgMTg6NTgsICJSb2IgTGFuZGxleSIgPHJvYkBsYW5kbGV5Lm5ldD4gd3Jv
dGU6DQoNCj5Gcm9tOiBSb2IgTGFuZGxleSA8cm9iQGxhbmRsZXkubmV0Pg0KPg0KPlRoZSBrZXJu
ZWwgaGFzIHN0cmluZyBwYXJzaW5nIGNvZGUgZm9yIE5GUyBtb3VudCBvcHRpb25zLCBidXQgaXQg
c2VlbXMNCj50byBoYXZlIGJpdC1yb3R0ZWQgb3ZlciB0aGUgeWVhcnMsIHNvIHRveWJveCBtb3Vu
dCBuZWVkcyB0aGUgZm9sbG93aW5nDQo+cGF0Y2ggdG8gYmUgYWJsZSB0byBtb3VudCBuZnMuIFdp
dGhvdXQgaXQsIHRoZSBrZXJuZWwgcmV0dXJucyAiaW52YWxpZA0KPmFyZ3VtZW50IiBiZWZvcmUg
c2VuZGluZyBhbnkgbmV0d29yayB0cmFmZmljLg0KPg0KPkZvciBtb3JlIGluZm9ybWF0aW9uLCBz
ZWUNCj5odHRwOi8vbGlzdHMubGFuZGxleS5uZXQvcGlwZXJtYWlsL3RveWJveC1sYW5kbGV5Lm5l
dC8yMDE2LU1hcmNoLzAwNDc5MC5odG1sDQo+DQo+U2lnbmVkLW9mZi1ieTogUm9iIExhbmRsZXkg
PHJvYkBsYW5kbGV5Lm5ldD4NCj4tLS0NCj4NCj4gZnMvbmZzL3N1cGVyLmMgfCAgIDE4ICsrKysr
KysrKysrKystLS0tLQ0KPiAxIGZpbGUgY2hhbmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgNSBkZWxl
dGlvbnMoLSkNCj4NCj5kaWZmIC0tZ2l0IGEvZnMvbmZzL3N1cGVyLmMgYi9mcy9uZnMvc3VwZXIu
Yw0KPmluZGV4IDIxMzdlMDIuLjg2MzU4NWQgMTAwNjQ0DQo+LS0tIGEvZnMvbmZzL3N1cGVyLmMN
Cj4rKysgYi9mcy9uZnMvc3VwZXIuYw0KPkBAIC0yMTMwLDExICsyMTMwLDIzIEBAIHN0YXRpYyBp
bnQgbmZzX3ZhbGlkYXRlX3RleHRfbW91bnRfZGF0YSh2b2lkICpvcHRpb25zLA0KPiAJaW50IHBv
cnQgPSAwOw0KPiAJaW50IG1heF9uYW1lbGVuID0gUEFHRV9TSVpFOw0KPiAJaW50IG1heF9wYXRo
bGVuID0gTkZTX01BWFBBVEhMRU47DQo+KwlpbnQgcmM7DQo+IAlzdHJ1Y3Qgc29ja2FkZHIgKnNh
cCA9IChzdHJ1Y3Qgc29ja2FkZHIgKikmYXJncy0+bmZzX3NlcnZlci5hZGRyZXNzOw0KPiANCj4g
CWlmIChuZnNfcGFyc2VfbW91bnRfb3B0aW9ucygoY2hhciAqKW9wdGlvbnMsIGFyZ3MpID09IDAp
DQo+IAkJcmV0dXJuIC1FSU5WQUw7DQo+IA0KPisJcmMgPSBuZnNfcGFyc2VfZGV2bmFtZShkZXZf
bmFtZSwNCj4rCQkJCSAgICZhcmdzLT5uZnNfc2VydmVyLmhvc3RuYW1lLA0KPisJCQkJICAgbWF4
X25hbWVsZW4sDQo+KwkJCQkgICAmYXJncy0+bmZzX3NlcnZlci5leHBvcnRfcGF0aCwNCj4rCQkJ
CSAgIG1heF9wYXRobGVuKTsNCj4rDQo+KwlhcmdzLT5uZnNfc2VydmVyLmFkZHJsZW4gPSBycGNf
cHRvbihhcmdzLT5uZXQsDQo+KwkJCWFyZ3MtPm5mc19zZXJ2ZXIuaG9zdG5hbWUsDQo+KwkJCXN0
cmxlbihhcmdzLT5uZnNfc2VydmVyLmhvc3RuYW1lKSwNCj4rCQkJc2FwLCBzaXplb2YoYXJncy0+
bmZzX3NlcnZlci5hZGRyZXNzKSk7DQo+Kw0KDQpUaGF0IHdpbGwgc2NyaWJibGUgb3ZlciB0aGUg
cGFyc2VkIGFkZHJlc3MuDQoNClRyb25kDQoNCg0K

Re: [PATCH] Fix NFS option parsing bit-rot.

[Rob Landley]

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:

Re: [PATCH] Fix NFS option parsing bit-rot.

[Trond Myklebust]

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

Re: [PATCH] Fix NFS option parsing bit-rot.

[Trond Myklebust]

DQoNCk9uIDYvOC8xNiwgMTU6NDEsICJSb2IgTGFuZGxleSIgPHJvYkBsYW5kbGV5Lm5ldD4gd3Jv
dGU6DQo+U28gYWRkcj0gaXMgYXQgX2Jlc3RfIHJlZHVuZGFudC4gWW91IE1VU1Qgc3VwcGx5IGhv
c3Q6LyBhbHdheXMsDQo+dGhlIGN1cnJlbnQgY29kZSBqdXN0IHdhc24ndCB1c2luZyBpdC4NCg0K
VGhhdOKAmXMgYmVjYXVzZSB5b3UganVzdCBoYXBwZW4gdG8gYmUgc3VwcGx5aW5nIGFuIElQIGFk
ZHJlc3MgaW5zdGVhZCBvZiBhIGhvc3RuYW1lLiBUaGUga2VybmVsIGhhcyBubyBETlMgcmVzb2x2
aW5nIGZ1bmN0aW9uYWxpdHkuIEl0IGNhbm5vdCByZXNvbHZlIGhvc3RuYW1lcyBpbnRvIElQIGFk
ZHJlc3NlcyB3aXRob3V0IGhlbHAgZWl0aGVyIGJ5IHRoZSBjYWxsZXIgb3IgYnkgbWVhbnMgb2Yg
YW4gdXBjYWxsLiBUaGF0IGlzIHdoeSDigJhhZGRyPeKAmSBoYXMgYmVlbiBhIG1hbmRhdG9yeSBw
YXJhbWV0ZXIgZXZlciBzaW5jZSB3ZSBpbnRyb2R1Y2VkIHRoZSB0ZXh0IGJhc2VkIHBhcnNlci4N
Cg0KTm93IHdlIGNvdWxkLCB0aGVvcmV0aWNhbGx5LCBoYXZlIHRoZSBjbGllbnQgY2FsbCBuZnNf
ZG5zX3Jlc29sdmVfbmFtZSgpIG9uIHRoZSBob3N0bmFtZSB0byByZXNvbHZlIGl0LiBIb3dldmVy
IHRoYXQgYnJlYWtzIHdoZW4geW91IGhhdmUgbmV0IG5hbWVzcGFjZXMgYW5kIHN1Y2gsIHNpbmNl
IHRoZSBrZXJuZWzigJlzIGRuc19xdWVyeSgpIGNhbGwgaXMgbm90IGNvbnRhaW5lciBhd2FyZS4N
Cg0KVHJvbmQNCg0K

Re: [PATCH] Fix NFS option parsing bit-rot.

[Rob Landley]

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

Re: [PATCH] Fix NFS option parsing bit-rot.

[Rob Landley]

On 06/08/2016 03:35 PM, Trond Myklebust wrote:
>=20
>=20
> On 6/8/16, 15:41, "Rob Landley" <rob@landley.net> wrote:
>>So addr=3D is at _best_ redundant. You MUST supply host:/ always,
>>the current code just wasn't using it.
>=20
> That=E2=80=99s because you just happen to be supplying an IP address inst=
ead 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 =E2=80=98addr=3D=E2=80=99 has been a m=
andatory
> parameter ever since we introduced the text based parser.

Then why is host: mandatory in the dev name?

Rob