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

["Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>]

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