From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Venkateswararao Jujjuri (JV)" Subject: Re: [V9fs-developer] [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk() Date: Tue, 24 Aug 2010 14:55:43 -0700 Message-ID: <4C743FDF.4000502@linux.vnet.ibm.com> References: <1282664585-12450-1-git-send-email-jvrao@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: v9fs-developer@lists.sourceforge.net, linux-fsdevel@vger.kernel.org To: Eric Van Hensbergen Return-path: Received: from e2.ny.us.ibm.com ([32.97.182.142]:53696 "EHLO e2.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756102Ab0HXVzp (ORCPT ); Tue, 24 Aug 2010 17:55:45 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e2.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id o7OLfRD2025958 for ; Tue, 24 Aug 2010 17:41:27 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o7OLtiTO096310 for ; Tue, 24 Aug 2010 17:55:44 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o7OLtiBP014210 for ; Tue, 24 Aug 2010 15:55:44 -0600 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Eric Van Hensbergen wrote: > On Tue, Aug 24, 2010 at 10:43 AM, Venkateswararao Jujjuri (JV) > wrote: >> NULL fid should be handled in cases where we endup calling v9fs_dir_release() >> before even we instantiate the fid in filp. This patch handles >> pasing a NULL p9_fid* to p9_client_clunk. >> >> Signed-off-by: Venkateswararao Jujjuri >> --- >> fs/9p/vfs_dir.c | 3 ++- >> net/9p/client.c | 3 +++ >> 2 files changed, 5 insertions(+), 1 deletions(-) >> >> diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c >> index 16c8a2a..5f08203 100644 >> --- a/fs/9p/vfs_dir.c >> +++ b/fs/9p/vfs_dir.c >> @@ -292,7 +292,8 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) >> >> fid = filp->private_data; >> P9_DPRINTK(P9_DEBUG_VFS, >> - "inode: %p filp: %p fid: %d\n", inode, filp, fid->fid); >> + "JV: inode: %p filp: %p fid: %d\n", inode, filp, >> + fid ? fid->fid : -1); > > Did you really mean to insert a JV: debug label in there? Oops!! > >> filemap_write_and_wait(inode->i_mapping); >> p9_client_clunk(fid); >> return 0; >> diff --git a/net/9p/client.c b/net/9p/client.c >> index dc6f2f2..9338fb3 100644 >> --- a/net/9p/client.c >> +++ b/net/9p/client.c >> @@ -1201,6 +1201,9 @@ int p9_client_clunk(struct p9_fid *fid) >> struct p9_client *clnt; >> struct p9_req_t *req; >> >> + if (!fid) >> + return 0; >> + >> > > While this will solve the NULL pointer dereference, it will do so > silently which may lead to us leaking fids/memory/resources/etc. If > we were to do such a thing, I'd want warning messages. However, I > wouldn't want warning messages in the generic, because now we have > places we are calling p9_client_clunk from where we expect null fids > to be valid. > > I'd suggest keeping the fid check in v9fs_dir_release to parameterize > sending the clunk since we expect to sometimes not have a fid here, > and then in a separate patch adding some code to p9_client_clunk which > complains loudly any time it gets called with a NULL fid. Its unclear > to me whether this should be a BUG() or just a warning, a warning > would probably suffice as it'll help us track down such cases during > testing without breaking users. So basically you need a conditional call to p9_client_clunk() . v9fs_dir_release() { ... if (fid) p9_client_clunk(); } Do you recall any cases where we end up calling clunk w/o a valid fid? So .. may be we should go with BUG(!fid) in clunck code? Thanks, JV > > -eric