* [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()
@ 2010-08-24 15:43 Venkateswararao Jujjuri (JV)
2010-08-24 19:38 ` [V9fs-developer] " Eric Van Hensbergen
0 siblings, 1 reply; 3+ messages in thread
From: Venkateswararao Jujjuri (JV) @ 2010-08-24 15:43 UTC (permalink / raw)
To: v9fs-developer; +Cc: linux-fsdevel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [V9fs-developer] [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()
2010-08-24 15:43 [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk() Venkateswararao Jujjuri (JV)
@ 2010-08-24 19:38 ` Eric Van Hensbergen
2010-08-24 21:55 ` Venkateswararao Jujjuri (JV)
0 siblings, 1 reply; 3+ messages in thread
From: Eric Van Hensbergen @ 2010-08-24 19:38 UTC (permalink / raw)
To: Venkateswararao Jujjuri (JV); +Cc: v9fs-developer, linux-fsdevel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [V9fs-developer] [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk()
2010-08-24 19:38 ` [V9fs-developer] " Eric Van Hensbergen
@ 2010-08-24 21:55 ` Venkateswararao Jujjuri (JV)
0 siblings, 0 replies; 3+ messages in thread
From: Venkateswararao Jujjuri (JV) @ 2010-08-24 21:55 UTC (permalink / raw)
To: Eric Van Hensbergen; +Cc: v9fs-developer, linux-fsdevel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-08-24 21:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-24 15:43 [PATCH] [fs/9p] Check for NULL fid pointers in p9_client_clunk() Venkateswararao Jujjuri (JV)
2010-08-24 19:38 ` [V9fs-developer] " Eric Van Hensbergen
2010-08-24 21:55 ` Venkateswararao Jujjuri (JV)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.