Eric Van Hensbergen wrote:
> On Tue, Aug 24, 2010 at 10:43 AM, Venkateswararao Jujjuri (JV)
> <jvrao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
