From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: NFS/credentials leak in 2.6.29-rc1 Date: Tue, 20 Jan 2009 18:53:41 -0500 Message-ID: <20090120235341.GA29017@fieldses.org> References: <20090120114649.GA15832@ioremap.net> <20090120151125.GB24266@fieldses.org> <20090120152304.GA28592@ioremap.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Trond Myklebust , linux-fsdevel@vger.kernel.org, David Howells To: Evgeniy Polyakov Return-path: Received: from mail.fieldses.org ([141.211.133.115]:39896 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755835AbZATXxm (ORCPT ); Tue, 20 Jan 2009 18:53:42 -0500 Content-Disposition: inline In-Reply-To: <20090120152304.GA28592@ioremap.net> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Jan 20, 2009 at 06:23:04PM +0300, Evgeniy Polyakov wrote: > On Tue, Jan 20, 2009 at 10:11:25AM -0500, J. Bruce Fields (bfields@fieldses.org) wrote: > > This doesn't look familiar, no; thanks for the report. I guess we > > should take a careful look at the recent changes to fs/nfsd/auth.c? > > If creds are allocated in nfsd_setuser() and never freed? groups_alloc() > can also explain size-256 slab grew, so this may be the place where > things are allocated, but why they are not freed? > This may also explain why I did not see this for the large sequential > IO, since number of requests to the server was noticebly smaller, than > in random IO test. Looking through nfsd_setuser(), one obvious bug: in the (flags & NFSEXP_ALLSQUASH) case, we never check the return value from the groups_alloc(0). If it returns NULL, we dereference it anyway. But that's unrelated. For the cred reference counting: - revert_creds(get_cred(current->real_cred)) modifies current->cred, putting the old value and getting the new. OK. - new = prepare_creds() creates a new object with count 1. All subsequent exits from the function go through the oom or error labels, which put new. OK. - Each of the three ALLSQUASH/ROOTSQUASH/else paths create a new reference to a groups_info struct in gi, which is unconditionally assigned to new by set_groups(). set_groups() takes its own reference on gi, then we put the original reference in the following put_group_info(). OK. - Finally, we put_cred(override_creds(new)). That modifies current->cred again, putting the old value and getting the new. Hm. But that last part's not OK; aren't we still holding our own reference to new, in addition to the one that override_creds() just took? So I think we need the following? --b. diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c index c903e04..9966e9e 100644 --- a/fs/nfsd/auth.c +++ b/fs/nfsd/auth.c @@ -85,6 +85,7 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp) new->cap_effective = cap_raise_nfsd_set(new->cap_effective, new->cap_permitted); put_cred(override_creds(new)); + put_cred(new); return 0; oom: