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@xxxxxxxxxx> wrote:
> On Wed, 2013-08-07 at 16:09 -0400, andros@xxxxxxxxxx wrote:
> > From: Andy Adamson <andros@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
www.netapp.com
From b72888cb0ba63b2dfc6c8d3cd78a7fea584bebc6 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---
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
