[PATCH v1 0/2] Improve construction of non-UCS

[PATCH v1 0/2] Improve construction of non-UCS

[Chuck Lever]

The following series proposes improvements to the default NFSv4.0
client ID strings. The purpose is to decrease the chance that two
distinct clients will choose the same string, and to make the same
client connecting via different transports choose the same string.

The use of the nodename comes from the NFSv4.1 (uniform) client
ID string. It has some shortcomings as well, but it is better
overall than using the client's local address. The use of the
uniquifier is also copied from the uniform client ID string.

The goal is to make the default behavior better. Preventing a
malicious administrator from choosing the same nodename, uniquifier,
and/or client address on purpose is not possible.

---

Chuck Lever (2):
      NFSv4.0: Remove cl_ipaddr from non-UCS client ID
      NFSv4.0: Remove transport protocol name from non-UCS client ID


 fs/nfs/nfs4proc.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

--
Chuck Lever

[PATCH v1 1/2] NFSv4.0: Remove cl_ipaddr from non-UCS client ID

[Chuck Lever]

It is possible for two distinct clients to have the same cl_ipaddr:

 - if the client admin disables callback with clientaddr=0.0.0.0 on
   more than one client

 - if two clients behind separate NATs use the same private subnet
   number

 - if the client admin specifies the same address via clientaddr=
   mount option (pointing the server at the same NAT box, for
   example)

Because of the way the Linux NFSv4.0 client constructs its client
ID string by default, such clients could interfere with each others'
lease state when mounting the same server:

	scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
		clp->cl_ipaddr,
		rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
		rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO));

cl_ipaddr is set to the value of the clientaddr= mount option. Two
clients whose addresses are 192.168.3.77 that mount the same server
(whose public IP address is, say, 3.4.5.6) would both generate the
same client ID string when sending a SETCLIENTID:

  Linux NFSv4.0 192.168.3.77/3.4.5.6 tcp

and thus the server would not be able to distinguish the clients'
leases. If both clients are using AUTH_SYS when sending SETCLIENTID
then the server could possibly permit the two clients to interfere
with or purge each others' leases.

To better ensure that Linux's NFSv4.0 client ID strings are distinct
in these cases, remove cl_ipaddr from the client ID string and
replace it with something more likely to be unique. Note that the
replacement looks a lot like the uniform client ID string.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b71757e..fa6f9a2 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5591,13 +5591,16 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
 		return 0;
 
 	rcu_read_lock();
-	len = 14 + strlen(clp->cl_ipaddr) + 1 +
+	len = 14 +
+		strlen(clp->cl_rpcclient->cl_nodename) +
+		1 +
 		strlen(rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR)) +
 		1 +
 		strlen(rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO)) +
 		1;
 	rcu_read_unlock();
-
+	if (nfs4_client_id_uniquifier[0] != '\0')
+		len += strlen(nfs4_client_id_uniquifier) + 1;
 	if (len > NFS4_OPAQUE_LIMIT + 1)
 		return -EINVAL;
 
@@ -5611,10 +5614,21 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
 		return -ENOMEM;
 
 	rcu_read_lock();
-	scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
-			clp->cl_ipaddr,
-			rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
-			rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO));
+	if (nfs4_client_id_uniquifier[0] != '\0')
+		scnprintf(str, len, "Linux NFSv4.0 %s/%s/%s %s",
+			  clp->cl_rpcclient->cl_nodename,
+			  nfs4_client_id_uniquifier,
+			  rpc_peeraddr2str(clp->cl_rpcclient,
+					   RPC_DISPLAY_ADDR),
+			  rpc_peeraddr2str(clp->cl_rpcclient,
+					   RPC_DISPLAY_PROTO));
+	else
+		scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
+			  clp->cl_rpcclient->cl_nodename,
+			  rpc_peeraddr2str(clp->cl_rpcclient,
+					   RPC_DISPLAY_ADDR),
+			  rpc_peeraddr2str(clp->cl_rpcclient,
+					   RPC_DISPLAY_PROTO));
 	rcu_read_unlock();
 
 	clp->cl_owner_id = str;

[PATCH v1 2/2] NFSv4.0: Remove transport protocol name from non-UCS client ID

[Chuck Lever]

Commit 69dd716c5ffd ("NFSv4: Add socket proto argument to
setclientid") (2007) added the transport protocol name to the client
ID string, but the patch description doesn't explain why this was
necessary.

At that time, the only transport protocol name that would have been
used is "tcp" (for both IPv4 and IPv6), resulting in no additional
distinctiveness of the client ID string.

Since there is one client instance, the server should recognize it's
state whether the client is connecting via TCP or RDMA. Same client,
same lease.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index fa6f9a2..aa07745 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5595,8 +5595,6 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
 		strlen(clp->cl_rpcclient->cl_nodename) +
 		1 +
 		strlen(rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR)) +
-		1 +
-		strlen(rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_PROTO)) +
 		1;
 	rcu_read_unlock();
 	if (nfs4_client_id_uniquifier[0] != '\0')
@@ -5615,20 +5613,16 @@ static void nfs4_init_boot_verifier(const struct nfs_client *clp,
 
 	rcu_read_lock();
 	if (nfs4_client_id_uniquifier[0] != '\0')
-		scnprintf(str, len, "Linux NFSv4.0 %s/%s/%s %s",
+		scnprintf(str, len, "Linux NFSv4.0 %s/%s/%s",
 			  clp->cl_rpcclient->cl_nodename,
 			  nfs4_client_id_uniquifier,
 			  rpc_peeraddr2str(clp->cl_rpcclient,
-					   RPC_DISPLAY_ADDR),
-			  rpc_peeraddr2str(clp->cl_rpcclient,
-					   RPC_DISPLAY_PROTO));
+					   RPC_DISPLAY_ADDR));
 	else
-		scnprintf(str, len, "Linux NFSv4.0 %s/%s %s",
+		scnprintf(str, len, "Linux NFSv4.0 %s/%s",
 			  clp->cl_rpcclient->cl_nodename,
 			  rpc_peeraddr2str(clp->cl_rpcclient,
-					   RPC_DISPLAY_ADDR),
-			  rpc_peeraddr2str(clp->cl_rpcclient,
-					   RPC_DISPLAY_PROTO));
+					   RPC_DISPLAY_ADDR));
 	rcu_read_unlock();
 
 	clp->cl_owner_id = str;

Fwd: [PATCH v1 0/2] Improve construction of non-UCS

[Chuck Lever]

Hi Trond-

> Begin forwarded message:
> 
> From: Chuck Lever <chuck.lever@oracle.com>
> Subject: [PATCH v1 0/2] Improve construction of non-UCS
> Date: June 4, 2018 at 10:53:23 AM EDT
> To: linux-nfs@vger.kernel.org
> 
> The following series proposes improvements to the default NFSv4.0
> client ID strings. The purpose is to decrease the chance that two
> distinct clients will choose the same string, and to make the same
> client connecting via different transports choose the same string.
> 
> The use of the nodename comes from the NFSv4.1 (uniform) client
> ID string. It has some shortcomings as well, but it is better
> overall than using the client's local address. The use of the
> uniquifier is also copied from the uniform client ID string.
> 
> The goal is to make the default behavior better. Preventing a
> malicious administrator from choosing the same nodename, uniquifier,
> and/or client address on purpose is not possible.
> 
> ---
> 
> Chuck Lever (2):
>      NFSv4.0: Remove cl_ipaddr from non-UCS client ID
>      NFSv4.0: Remove transport protocol name from non-UCS client ID
> 
> 
> fs/nfs/nfs4proc.c |   24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)

Are you building the official NFS client PR for this merge window?
Would you consider taking this series?


--
Chuck Lever

Re: [PATCH v1 2/2] NFSv4.0: Remove transport protocol name from non-UCS client ID

[Trond Myklebust]

T24gTW9uLCAyMDE4LTA2LTA0IGF0IDEwOjUzIC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
Q29tbWl0IDY5ZGQ3MTZjNWZmZCAoIk5GU3Y0OiBBZGQgc29ja2V0IHByb3RvIGFyZ3VtZW50IHRv
DQo+IHNldGNsaWVudGlkIikgKDIwMDcpIGFkZGVkIHRoZSB0cmFuc3BvcnQgcHJvdG9jb2wgbmFt
ZSB0byB0aGUgY2xpZW50DQo+IElEIHN0cmluZywgYnV0IHRoZSBwYXRjaCBkZXNjcmlwdGlvbiBk
b2Vzbid0IGV4cGxhaW4gd2h5IHRoaXMgd2FzDQo+IG5lY2Vzc2FyeS4NCj4gDQo+IEF0IHRoYXQg
dGltZSwgdGhlIG9ubHkgdHJhbnNwb3J0IHByb3RvY29sIG5hbWUgdGhhdCB3b3VsZCBoYXZlIGJl
ZW4NCj4gdXNlZCBpcyAidGNwIiAoZm9yIGJvdGggSVB2NCBhbmQgSVB2NiksIHJlc3VsdGluZyBp
biBubyBhZGRpdGlvbmFsDQo+IGRpc3RpbmN0aXZlbmVzcyBvZiB0aGUgY2xpZW50IElEIHN0cmlu
Zy4NCj4gDQo+IFNpbmNlIHRoZXJlIGlzIG9uZSBjbGllbnQgaW5zdGFuY2UsIHRoZSBzZXJ2ZXIg
c2hvdWxkIHJlY29nbml6ZSBpdCdzDQo+IHN0YXRlIHdoZXRoZXIgdGhlIGNsaWVudCBpcyBjb25u
ZWN0aW5nIHZpYSBUQ1Agb3IgUkRNQS4gU2FtZSBjbGllbnQsDQo+IHNhbWUgbGVhc2UuDQoNClRo
ZSByZWFzb24gd2h5IHRoaXMgaXMgdGhlIGNhc2Ugbm93IGlzIGJlY2F1c2UgdGhlIHRydW5raW5n
IGNvZGUNCm92ZXJyaWRlcyB0aGUgZ3VhcmRyYWlscyBpbiBuZnNfZ2V0X2NsaWVudCgpLiBUaGUg
bGF0dGVyIGRvZXMgbWF0Y2ggb24NCnRoZSBwcm90b2NvbC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1
c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgSGFtbWVyc3BhY2UNCnRyb25kLm15a2xl
YnVzdEBoYW1tZXJzcGFjZS5jb20NCg0K

Re: [PATCH v1 2/2] NFSv4.0: Remove transport protocol name from non-UCS client ID

[Chuck Lever]

> On Jun 6, 2018, at 11:44 AM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Mon, 2018-06-04 at 10:53 -0400, Chuck Lever wrote:
>> Commit 69dd716c5ffd ("NFSv4: Add socket proto argument to
>> setclientid") (2007) added the transport protocol name to the client
>> ID string, but the patch description doesn't explain why this was
>> necessary.
>>=20
>> At that time, the only transport protocol name that would have been
>> used is "tcp" (for both IPv4 and IPv6), resulting in no additional
>> distinctiveness of the client ID string.
>>=20
>> Since there is one client instance, the server should recognize it's
>> state whether the client is connecting via TCP or RDMA. Same client,
>> same lease.
>=20
> The reason why this is the case now is because the trunking code
> overrides the guardrails in nfs_get_client(). The latter does match on
> the protocol.

OK. Would that also true in the UCS case?


--
Chuck Lever

Re: [PATCH v1 2/2] NFSv4.0: Remove transport protocol name from non-UCS client ID

[Trond Myklebust]

T24gV2VkLCAyMDE4LTA2LTA2IGF0IDExOjQ4IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
PiBPbiBKdW4gNiwgMjAxOCwgYXQgMTE6NDQgQU0sIFRyb25kIE15a2xlYnVzdCA8dHJvbmRteUBo
YW1tZXJzcGFjZS5jDQo+ID4gb20+IHdyb3RlOg0KPiA+IA0KPiA+IE9uIE1vbiwgMjAxOC0wNi0w
NCBhdCAxMDo1MyAtMDQwMCwgQ2h1Y2sgTGV2ZXIgd3JvdGU6DQo+ID4gPiBDb21taXQgNjlkZDcx
NmM1ZmZkICgiTkZTdjQ6IEFkZCBzb2NrZXQgcHJvdG8gYXJndW1lbnQgdG8NCj4gPiA+IHNldGNs
aWVudGlkIikgKDIwMDcpIGFkZGVkIHRoZSB0cmFuc3BvcnQgcHJvdG9jb2wgbmFtZSB0byB0aGUN
Cj4gPiA+IGNsaWVudA0KPiA+ID4gSUQgc3RyaW5nLCBidXQgdGhlIHBhdGNoIGRlc2NyaXB0aW9u
IGRvZXNuJ3QgZXhwbGFpbiB3aHkgdGhpcyB3YXMNCj4gPiA+IG5lY2Vzc2FyeS4NCj4gPiA+IA0K
PiA+ID4gQXQgdGhhdCB0aW1lLCB0aGUgb25seSB0cmFuc3BvcnQgcHJvdG9jb2wgbmFtZSB0aGF0
IHdvdWxkIGhhdmUNCj4gPiA+IGJlZW4NCj4gPiA+IHVzZWQgaXMgInRjcCIgKGZvciBib3RoIElQ
djQgYW5kIElQdjYpLCByZXN1bHRpbmcgaW4gbm8NCj4gPiA+IGFkZGl0aW9uYWwNCj4gPiA+IGRp
c3RpbmN0aXZlbmVzcyBvZiB0aGUgY2xpZW50IElEIHN0cmluZy4NCj4gPiA+IA0KPiA+ID4gU2lu
Y2UgdGhlcmUgaXMgb25lIGNsaWVudCBpbnN0YW5jZSwgdGhlIHNlcnZlciBzaG91bGQgcmVjb2du
aXplDQo+ID4gPiBpdCdzDQo+ID4gPiBzdGF0ZSB3aGV0aGVyIHRoZSBjbGllbnQgaXMgY29ubmVj
dGluZyB2aWEgVENQIG9yIFJETUEuIFNhbWUNCj4gPiA+IGNsaWVudCwNCj4gPiA+IHNhbWUgbGVh
c2UuDQo+ID4gDQo+ID4gVGhlIHJlYXNvbiB3aHkgdGhpcyBpcyB0aGUgY2FzZSBub3cgaXMgYmVj
YXVzZSB0aGUgdHJ1bmtpbmcgY29kZQ0KPiA+IG92ZXJyaWRlcyB0aGUgZ3VhcmRyYWlscyBpbiBu
ZnNfZ2V0X2NsaWVudCgpLiBUaGUgbGF0dGVyIGRvZXMgbWF0Y2gNCj4gPiBvbg0KPiA+IHRoZSBw
cm90b2NvbC4NCj4gDQo+IE9LLiBXb3VsZCB0aGF0IGFsc28gdHJ1ZSBpbiB0aGUgVUNTIGNhc2U/
DQo+IA0KDQpZZXMsIEkgYmVsaWV2ZSB0aGF0IGNvZGUgaXMgaW5kZXBlbmRlbnQgb2YgdGhlIG1p
Z3JhdGlvbiBmbGFnLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBt
YWludGFpbmVyLCBIYW1tZXJzcGFjZQ0KdHJvbmQubXlrbGVidXN0QGhhbW1lcnNwYWNlLmNvbQ0K
DQo=

Re: [PATCH v1 2/2] NFSv4.0: Remove transport protocol name from non-UCS client ID

[Chuck Lever]

> On Jun 6, 2018, at 11:55 AM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Wed, 2018-06-06 at 11:48 -0400, Chuck Lever wrote:
>>> On Jun 6, 2018, at 11:44 AM, Trond Myklebust <trondmy@hammerspace.c
>>> om> wrote:
>>>=20
>>> On Mon, 2018-06-04 at 10:53 -0400, Chuck Lever wrote:
>>>> Commit 69dd716c5ffd ("NFSv4: Add socket proto argument to
>>>> setclientid") (2007) added the transport protocol name to the
>>>> client
>>>> ID string, but the patch description doesn't explain why this was
>>>> necessary.
>>>>=20
>>>> At that time, the only transport protocol name that would have
>>>> been
>>>> used is "tcp" (for both IPv4 and IPv6), resulting in no
>>>> additional
>>>> distinctiveness of the client ID string.
>>>>=20
>>>> Since there is one client instance, the server should recognize
>>>> it's
>>>> state whether the client is connecting via TCP or RDMA. Same
>>>> client,
>>>> same lease.
>>>=20
>>> The reason why this is the case now is because the trunking code
>>> overrides the guardrails in nfs_get_client(). The latter does match
>>> on
>>> the protocol.
>>=20
>> OK. Would that also true in the UCS case?
>>=20
>=20
> Yes, I believe that code is independent of the migration flag.

But the UCS does not include the transport type. Maybe something is
broken here (that something could be my understanding). Seems to me
that could impact transparent state migration between a server that
is mounted with RDMA, and one that has only TCP.

I'm willing to drop 2/2.


--
Chuck Lever