From patchwork Thu Aug 8 00:58:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 2840686 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 1342FBF535 for ; Thu, 8 Aug 2013 00:58:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 57C6F20488 for ; Thu, 8 Aug 2013 00:58:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6D67920483 for ; Thu, 8 Aug 2013 00:58:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933623Ab3HHA6h (ORCPT ); Wed, 7 Aug 2013 20:58:37 -0400 Received: from mx12.netapp.com ([216.240.18.77]:21747 "EHLO mx12.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933587Ab3HHA6h (ORCPT ); Wed, 7 Aug 2013 20:58:37 -0400 X-IronPort-AV: E=Sophos;i="4.89,835,1367996400"; d="scan'208,223";a="79477125" Received: from vmwexceht01-prd.hq.netapp.com ([10.106.76.239]) by mx12-out.netapp.com with ESMTP; 07 Aug 2013 17:58:36 -0700 Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.252]) by vmwexceht01-prd.hq.netapp.com ([10.106.76.239]) with mapi id 14.03.0123.003; Wed, 7 Aug 2013 17:58:36 -0700 From: "Myklebust, Trond" To: Andy Adamson CC: "Adamson, Andy" , "linux-nfs@vger.kernel.org" Subject: Re: [PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak Thread-Topic: [PATCH Version 2 1/6] NFSv4.1 Fix an rpc client leak Thread-Index: AQHOk6oX2JWMXcxwg0GU1w/I+BAXIJmKqH+AgAAPEACAADtEgA== Date: Thu, 8 Aug 2013 00:58:35 +0000 Message-ID: <1375923513.26965.2.camel@leira.trondhjem.org> References: <1375906192-1988-1-git-send-email-andros@netapp.com> <1375907551.7280.57.camel@leira.trondhjem.org> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-originating-ip: [10.106.53.51] MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 2013-08-07 at 17:26 -0400, Andy Adamson wrote: > > > > On Wed, Aug 7, 2013 at 4:32 PM, Myklebust, Trond > wrote: > On Wed, 2013-08-07 at 16:09 -0400, andros@netapp.com wrote: > > From: Andy Adamson > > > > 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 > > --- > > 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 From b72888cb0ba63b2dfc6c8d3cd78a7fea584bebc6 Mon Sep 17 00:00:00 2001 From: Trond Myklebust 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 Signed-off-by: Trond Myklebust --- 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