[PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak

[PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak

[andros]

From: Andy Adamson <andros@netapp.com>

nfs4_proc_lookup_mountpoint clones an rpc client to perform a lookup across
a mountpoint. If the security of the mountpoint is different than that of
the clonded rpc client, a secinfo query is sent, and if successful, a new
rpc client is cloned an returned to nfs4_proc_lookup_mountpoint replacing
the original cloned client pointer. In this case, the originoal cloned client
is not reaped - which results in a leak of multiple rpc clients, as the
parent of the original cloned client will not be dereferenced, and it's
parent will not be dereferenced...

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0e64ccc..ee30a4f 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3073,12 +3073,15 @@ nfs4_proc_lookup_mountpoint(struct inode *dir, struct qstr *name,
 {
 	int status;
 	struct rpc_clnt *client = rpc_clone_client(NFS_CLIENT(dir));
+	struct rpc_clnt *clnt = client;
 
 	status = nfs4_proc_lookup_common(&client, dir, name, fhandle, fattr, NULL);
 	if (status < 0) {
 		rpc_shutdown_client(client);
 		return ERR_PTR(status);
 	}
+	if (clnt != client)
+		rpc_shutdown_client(clnt);
 	return client;
 }
 
-- 
1.8.3.1

Re: [PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak

[Myklebust, Trond]

T24gV2VkLCAyMDEzLTA4LTA3IGF0IDE2OjA5IC0wNDAwLCBhbmRyb3NAbmV0YXBwLmNvbSB3cm90
ZToNCj4gRnJvbTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gDQo+IG5mczRf
cHJvY19sb29rdXBfbW91bnRwb2ludCBjbG9uZXMgYW4gcnBjIGNsaWVudCB0byBwZXJmb3JtIGEg
bG9va3VwIGFjcm9zcw0KPiBhIG1vdW50cG9pbnQuIElmIHRoZSBzZWN1cml0eSBvZiB0aGUgbW91
bnRwb2ludCBpcyBkaWZmZXJlbnQgdGhhbiB0aGF0IG9mDQo+IHRoZSBjbG9uZGVkIHJwYyBjbGll
bnQsIGEgc2VjaW5mbyBxdWVyeSBpcyBzZW50LCBhbmQgaWYgc3VjY2Vzc2Z1bCwgYSBuZXcNCj4g
cnBjIGNsaWVudCBpcyBjbG9uZWQgYW4gcmV0dXJuZWQgdG8gbmZzNF9wcm9jX2xvb2t1cF9tb3Vu
dHBvaW50IHJlcGxhY2luZw0KPiB0aGUgb3JpZ2luYWwgY2xvbmVkIGNsaWVudCBwb2ludGVyLiBJ
biB0aGlzIGNhc2UsIHRoZSBvcmlnaW5vYWwgY2xvbmVkIGNsaWVudA0KPiBpcyBub3QgcmVhcGVk
IC0gd2hpY2ggcmVzdWx0cyBpbiBhIGxlYWsgb2YgbXVsdGlwbGUgcnBjIGNsaWVudHMsIGFzIHRo
ZQ0KPiBwYXJlbnQgb2YgdGhlIG9yaWdpbmFsIGNsb25lZCBjbGllbnQgd2lsbCBub3QgYmUgZGVy
ZWZlcmVuY2VkLCBhbmQgaXQncw0KPiBwYXJlbnQgd2lsbCBub3QgYmUgZGVyZWZlcmVuY2VkLi4u
DQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBBbmR5IEFkYW1zb24gPGFuZHJvc0BuZXRhcHAuY29tPg0K
PiAtLS0NCj4gIGZzL25mcy9uZnM0cHJvYy5jIHwgMyArKysNCj4gIDEgZmlsZSBjaGFuZ2VkLCAz
IGluc2VydGlvbnMoKykNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHByb2MuYyBiL2Zz
L25mcy9uZnM0cHJvYy5jDQo+IGluZGV4IDBlNjRjY2MuLmVlMzBhNGYgMTAwNjQ0DQo+IC0tLSBh
L2ZzL25mcy9uZnM0cHJvYy5jDQo+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+IEBAIC0zMDcz
LDEyICszMDczLDE1IEBAIG5mczRfcHJvY19sb29rdXBfbW91bnRwb2ludChzdHJ1Y3QgaW5vZGUg
KmRpciwgc3RydWN0IHFzdHIgKm5hbWUsDQo+ICB7DQo+ICAJaW50IHN0YXR1czsNCj4gIAlzdHJ1
Y3QgcnBjX2NsbnQgKmNsaWVudCA9IHJwY19jbG9uZV9jbGllbnQoTkZTX0NMSUVOVChkaXIpKTsN
Cj4gKwlzdHJ1Y3QgcnBjX2NsbnQgKmNsbnQgPSBjbGllbnQ7DQo+ICANCj4gIAlzdGF0dXMgPSBu
ZnM0X3Byb2NfbG9va3VwX2NvbW1vbigmY2xpZW50LCBkaXIsIG5hbWUsIGZoYW5kbGUsIGZhdHRy
LCBOVUxMKTsNCj4gIAlpZiAoc3RhdHVzIDwgMCkgew0KPiAgCQlycGNfc2h1dGRvd25fY2xpZW50
KGNsaWVudCk7DQo+ICAJCXJldHVybiBFUlJfUFRSKHN0YXR1cyk7DQo+ICAJfQ0KPiArCWlmIChj
bG50ICE9IGNsaWVudCkNCj4gKwkJcnBjX3NodXRkb3duX2NsaWVudChjbG50KTsNCj4gIAlyZXR1
cm4gY2xpZW50Ow0KPiAgfQ0KPiAgDQpXb3VsZG4ndCB0aGF0IHNodXQgZG93biB0aGUgY2xpZW50
IHRoYXQgc3RpbGwgYmVsb25ncyB0bw0KTkZTX1NFUlZFUihkaXIpPw0KDQoNCi0tIA0KVHJvbmQg
TXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5N
eWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg==

Re: [PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

On Wed, 2013-08-07 at 17:26 -0400, Andy Adamson wrote:
> 
> 
> 
> On Wed, Aug 7, 2013 at 4:32 PM, Myklebust, Trond
> <Trond.Myklebust@netapp.com> wrote:
>         On Wed, 2013-08-07 at 16:09 -0400, andros@netapp.com wrote:
>         > From: Andy Adamson <andros@netapp.com>
>         >
>         > nfs4_proc_lookup_mountpoint clones an rpc client to perform
>         a lookup across
>         > a mountpoint. If the security of the mountpoint is different
>         than that of
>         > the clonded rpc client, a secinfo query is sent, and if
>         successful, a new
>         > rpc client is cloned an returned to
>         nfs4_proc_lookup_mountpoint replacing
>         > the original cloned client pointer. In this case, the
>         originoal cloned client
>         > is not reaped - which results in a leak of multiple rpc
>         clients, as the
>         > parent of the original cloned client will not be
>         dereferenced, and it's
>         > parent will not be dereferenced...
>         >
>         > Signed-off-by: Andy Adamson <andros@netapp.com>
>         > ---
>         >  fs/nfs/nfs4proc.c | 3 +++
>         >  1 file changed, 3 insertions(+)
>         >
>         > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>         > index 0e64ccc..ee30a4f 100644
>         > --- a/fs/nfs/nfs4proc.c
>         > +++ b/fs/nfs/nfs4proc.c
>         > @@ -3073,12 +3073,15 @@ nfs4_proc_lookup_mountpoint(struct
>         inode *dir, struct qstr *name,
>         >  {
>         >       int status;
>         >       struct rpc_clnt *client =
>         rpc_clone_client(NFS_CLIENT(dir));
>         > +     struct rpc_clnt *clnt = client;
>         >
>         >       status = nfs4_proc_lookup_common(&client, dir, name,
>         fhandle, fattr, NULL);
>         >       if (status < 0) {
>         >               rpc_shutdown_client(client);
>         >               return ERR_PTR(status);
>         >       }
>         > +     if (clnt != client)
>         > +             rpc_shutdown_client(clnt);
>         >       return client;
>         >  }
>         > 
>         
>         Wouldn't that shut down the client that still belongs to
>         NFS_SERVER(dir)?
> 
> 
> No. It shuts down a _clone_ of the client that still belongs to
> NFS_SERVER(dir).
> 

OK. However how about the case where rpc_clone_client() returns an
error? As far as I can tell neither the fix nor the original code deals
with that case.
How about something like the following, where we defer the clone until
we know that it might be needed?

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-up-nfs4_proc_lookup_mountpoint.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-up-nfs4_proc_lookup_mountpoint.patch", Size: 1659 bytes --]

From b72888cb0ba63b2dfc6c8d3cd78a7fea584bebc6 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 7 Aug 2013 20:38:07 -0400
Subject: [PATCH] NFSv4: Fix up nfs4_proc_lookup_mountpoint

Currently, we do not check the return value of client = rpc_clone_client(),
nor do we shut down the resulting cloned rpc_clnt in the case where a
NFS4ERR_WRONGSEC has caused nfs4_proc_lookup_common() to replace the
original value of 'client' (causing a memory leak).

Fix both issues and simplify the code by moving the call to
rpc_clone_client() until after nfs4_proc_lookup_common() has
done its business.

Reported-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4proc.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index cf11799..108a774 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3071,15 +3071,13 @@ struct rpc_clnt *
 nfs4_proc_lookup_mountpoint(struct inode *dir, struct qstr *name,
 			    struct nfs_fh *fhandle, struct nfs_fattr *fattr)
 {
+	struct rpc_clnt *client = NFS_CLIENT(dir);
 	int status;
-	struct rpc_clnt *client = rpc_clone_client(NFS_CLIENT(dir));
 
 	status = nfs4_proc_lookup_common(&client, dir, name, fhandle, fattr, NULL);
-	if (status < 0) {
-		rpc_shutdown_client(client);
+	if (status < 0)
 		return ERR_PTR(status);
-	}
-	return client;
+	return (client == NFS_CLIENT(dir)) ? rpc_clone_client(client) : client;
 }
 
 static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry)
-- 
1.8.3.1

Re: [PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak

[Adamson, Andy]

On Aug 7, 2013, at 8:58 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com>
 wrote:

> On Wed, 2013-08-07 at 17:26 -0400, Andy Adamson wrote:
>> 
>> 
>> 
>> On Wed, Aug 7, 2013 at 4:32 PM, Myklebust, Trond
>> <Trond.Myklebust@netapp.com> wrote:
>>        On Wed, 2013-08-07 at 16:09 -0400, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>> 
>>> nfs4_proc_lookup_mountpoint clones an rpc client to perform
>>        a lookup across
>>> a mountpoint. If the security of the mountpoint is different
>>        than that of
>>> the clonded rpc client, a secinfo query is sent, and if
>>        successful, a new
>>> rpc client is cloned an returned to
>>        nfs4_proc_lookup_mountpoint replacing
>>> the original cloned client pointer. In this case, the
>>        originoal cloned client
>>> is not reaped - which results in a leak of multiple rpc
>>        clients, as the
>>> parent of the original cloned client will not be
>>        dereferenced, and it's
>>> parent will not be dereferenced...
>>> 
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfs/nfs4proc.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 0e64ccc..ee30a4f 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -3073,12 +3073,15 @@ nfs4_proc_lookup_mountpoint(struct
>>        inode *dir, struct qstr *name,
>>> {
>>>      int status;
>>>      struct rpc_clnt *client =
>>        rpc_clone_client(NFS_CLIENT(dir));
>>> +     struct rpc_clnt *clnt = client;
>>> 
>>>      status = nfs4_proc_lookup_common(&client, dir, name,
>>        fhandle, fattr, NULL);
>>>      if (status < 0) {
>>>              rpc_shutdown_client(client);
>>>              return ERR_PTR(status);
>>>      }
>>> +     if (clnt != client)
>>> +             rpc_shutdown_client(clnt);
>>>      return client;
>>> }
>>> 
>> 
>>        Wouldn't that shut down the client that still belongs to
>>        NFS_SERVER(dir)?
>> 
>> 
>> No. It shuts down a _clone_ of the client that still belongs to
>> NFS_SERVER(dir).
>> 
> 
> OK. However how about the case where rpc_clone_client() returns an
> error? As far as I can tell neither the fix nor the original code deals
> with that case.
> How about something like the following, where we defer the clone until
> we know that it might be needed?

Looks good. Testing verifies it works.

-->Andy

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> <0001-NFSv4-Fix-up-nfs4_proc_lookup_mountpoint.patch>