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?
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
