[PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()

[PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()

[Venkateswararao Jujjuri (JV)]

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 <jvrao@linux.vnet.ibm.com>
---
 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);
 	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;
+
 	P9_DPRINTK(P9_DEBUG_9P, ">>> TCLUNK fid %d\n", fid->fid);
 	err = 0;
 	clnt = fid->clnt;
-- 
1.6.5.2

Re: [V9fs-developer] [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()

[Eric Van Hensbergen]

On Tue, Aug 24, 2010 at 10:43 AM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> 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 <jvrao@linux.vnet.ibm.com>
> ---
>  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?

>        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.

       -eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V9fs-developer] [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()

[Venkateswararao Jujjuri (JV)]

Eric Van Hensbergen wrote:
> On Tue, Aug 24, 2010 at 10:43 AM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> 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 <jvrao@linux.vnet.ibm.com>
>> ---
>>  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