9pfs double kfree

9pfs double kfree

[Dave Jones]

Probably the first of many found with Coverity.

This is kfree'd outside of both arms of the if condition already,
so fall through and free it just once.

Second variant is double-nasty, it deref's the free'd fcall
before it tries to free it a second time.

(I wish we had a kfree variant that NULL'd the target when it was free'd)

Coverity bugs: 987, 986

Signed-off-by: Dave Jones <davej@redhat.com>


--- linux-2.6.15.noarch/fs/9p/vfs_super.c~	2006-03-06 01:53:38.000000000 -0500
+++ linux-2.6.15.noarch/fs/9p/vfs_super.c	2006-03-06 01:54:36.000000000 -0500
@@ -156,7 +156,6 @@ static struct super_block *v9fs_get_sb(s
 	stat_result = v9fs_t_stat(v9ses, newfid, &fcall);
 	if (stat_result < 0) {
 		dprintk(DEBUG_ERROR, "stat error\n");
-		kfree(fcall);
 		v9fs_t_clunk(v9ses, newfid);
 	} else {
 		/* Setup the Root Inode */
--- linux-2.6.15.noarch/fs/9p/vfs_inode.c~	2006-03-06 01:57:05.000000000 -0500
+++ linux-2.6.15.noarch/fs/9p/vfs_inode.c	2006-03-06 01:58:05.000000000 -0500
@@ -274,7 +274,6 @@ v9fs_create(struct v9fs_session_info *v9
 		PRINT_FCALL_ERROR("clone error", fcall);
 		goto error;
 	}
-	kfree(fcall);
 
 	err = v9fs_t_create(v9ses, fid, name, perm, mode, &fcall);
 	if (err < 0) {

Re: 9pfs double kfree

[David S. Miller]

From: Dave Jones <davej@redhat.com>
Date: Mon, 6 Mar 2006 02:04:58 -0500

> (I wish we had a kfree variant that NULL'd the target when it was free'd)

Excellent idea.

Re: 9pfs double kfree

[Al Viro]

On Sun, Mar 05, 2006 at 11:07:11PM -0800, David S. Miller wrote:
> From: Dave Jones <davej@redhat.com>
> Date: Mon, 6 Mar 2006 02:04:58 -0500
> 
> > (I wish we had a kfree variant that NULL'd the target when it was free'd)
> 
> Excellent idea.

ITYM "poison the pointer variable".  Otherwise that sort of crap will go
unnoticed.

Re: 9pfs double kfree

[Balbir Singh]

On 3/6/06, David S. Miller <davem@davemloft.net> wrote:
> From: Dave Jones <davej@redhat.com>
> Date: Mon, 6 Mar 2006 02:04:58 -0500
>
> > (I wish we had a kfree variant that NULL'd the target when it was free'd)
>
> Excellent idea.
> -

Slab debugging should catch double frees, but it will not attract your
attention till you see your dmesg log. kfree() will ignore NULL
pointer, from the comments in kfree

/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
 *
  * If @objp is NULL, no operation is performed.
<snip>

May we could have such a variant under CONFIG_DEBUG_SLAB if we needed
and also change the variant kfree to BUG_ON() a NULL pointer.

Balbir

Re: 9pfs double kfree

[Dave Jones]

On Mon, Mar 06, 2006 at 07:23:46AM +0000, Al Viro wrote:
 > On Sun, Mar 05, 2006 at 11:07:11PM -0800, David S. Miller wrote:
 > > From: Dave Jones <davej@redhat.com>
 > > Date: Mon, 6 Mar 2006 02:04:58 -0500
 > > 
 > > > (I wish we had a kfree variant that NULL'd the target when it was free'd)
 > > 
 > > Excellent idea.
 > 
 > ITYM "poison the pointer variable".  Otherwise that sort of crap will go
 > unnoticed.

yeah, even better idea. Just like slab debug, but without the overhead
of poisoning the whole object.

I wonder if we could get away with something as simple as..

#define kfree(foo) \
	__kfree(foo); \
	foo = KFREE_POISON;

?

		Dave

-- 
http://www.codemonkey.org.uk

Re: 9pfs double kfree

[Dave Jones]

On Mon, Mar 06, 2006 at 12:56:03PM +0530, Balbir Singh wrote:
 > On 3/6/06, David S. Miller <davem@davemloft.net> wrote:
 > > From: Dave Jones <davej@redhat.com>
 > > Date: Mon, 6 Mar 2006 02:04:58 -0500
 > >
 > > > (I wish we had a kfree variant that NULL'd the target when it was free'd)
 > >
 > > Excellent idea.
 > > -
 > 
 > Slab debugging should catch double frees, but it will not attract your
 > attention till you see your dmesg log.

The vast majority of people never run with slab poisoning enabled
judging by the number of bugs it constantly turns up.

 > kfree() will ignore NULL
 > pointer, from the comments in kfree

*nod*, poisoning the ptr would be a better idea.

 > May we could have such a variant under CONFIG_DEBUG_SLAB if we needed
 > and also change the variant kfree to BUG_ON() a NULL pointer.

given the cost is just a ptr assignment in a slow path, I'd prefer
it was non-optional, otherwise it'll be as underused as the other
debugging options.

		Dave

-- 
http://www.codemonkey.org.uk

Re: 9pfs double kfree

[Balbir Singh]

> The vast majority of people never run with slab poisoning enabled
> judging by the number of bugs it constantly turns up.
>

Agreed, maybe we could insist that developers enable these options and
test their patch with debugging enabled before posting them, but I
have a feeling that it would meet a lot of resistance :-). Doesn't
hurt to document it in Documentation/HOWTO or somewhere more
appropriate.

>  > kfree() will ignore NULL
>  > pointer, from the comments in kfree
>
> *nod*, poisoning the ptr would be a better idea.
>

Yes, I think this is a better idea.

>  > May we could have such a variant under CONFIG_DEBUG_SLAB if we needed
>  > and also change the variant kfree to BUG_ON() a NULL pointer.
>
> given the cost is just a ptr assignment in a slow path, I'd prefer
> it was non-optional, otherwise it'll be as underused as the other
> debugging options.
>

Yes, agreed.

Re: 9pfs double kfree

[Pekka Enberg]

On 3/6/06, Dave Jones <davej@redhat.com> wrote:
> I wonder if we could get away with something as simple as..
>
> #define kfree(foo) \
>         __kfree(foo); \
>         foo = KFREE_POISON;
>
> ?

It's legal to call kfree() twice for NULL pointer. The above poisons
foo unconditionally which makes that case break I think.

                                Pekka

Re: 9pfs double kfree

[Dave Jones]

On Mon, Mar 06, 2006 at 09:56:22AM +0200, Pekka Enberg wrote:
 > On 3/6/06, Dave Jones <davej@redhat.com> wrote:
 > > I wonder if we could get away with something as simple as..
 > >
 > > #define kfree(foo) \
 > >         __kfree(foo); \
 > >         foo = KFREE_POISON;
 > >
 > > ?
 > 
 > It's legal to call kfree() twice for NULL pointer. The above poisons
 > foo unconditionally which makes that case break I think.

Bah, I never did like that rule.

		Dave

-- 
http://www.codemonkey.org.uk

Re: 9pfs double kfree

[Al Viro]

On Mon, Mar 06, 2006 at 09:56:22AM +0200, Pekka Enberg wrote:
> On 3/6/06, Dave Jones <davej@redhat.com> wrote:
> > I wonder if we could get away with something as simple as..
> >
> > #define kfree(foo) \
> >         __kfree(foo); \
> >         foo = KFREE_POISON;
> >
> > ?
> 
> It's legal to call kfree() twice for NULL pointer. The above poisons
> foo unconditionally which makes that case break I think.

Legal, but rather bad taste.  Init to NULL, possibly assign the value
if kmalloc(), then kfree() unconditionally - sure, but that... almost
certainly one hell of a lousy cleanup logics somewhere.

There's worse problem with that, though:
	kfree(container_of(......));
and it simply won't compile.

Re: 9pfs double kfree

[Pekka Enberg]

On 3/6/06, Al Viro <viro@ftp.linux.org.uk> wrote:
> Legal, but rather bad taste.  Init to NULL, possibly assign the value
> if kmalloc(), then kfree() unconditionally - sure, but that... almost
> certainly one hell of a lousy cleanup logics somewhere.

Agreed.

                                 Pekka

Re: 9pfs double kfree

[Arjan van de Ven]

On Mon, 2006-03-06 at 10:23 +0200, Pekka Enberg wrote:
> On 3/6/06, Al Viro <viro@ftp.linux.org.uk> wrote:
> > Legal, but rather bad taste.  Init to NULL, possibly assign the value
> > if kmalloc(), then kfree() unconditionally - sure, but that... almost
> > certainly one hell of a lousy cleanup logics somewhere.
> 
> Agreed.

btw slab already detects double free()s. That doesn't mean this idea is
useless; it'll catch stuff, but it's not for pure finding double free ;)

Re: 9pfs double kfree

[Kai Makisara]

On Mon, 6 Mar 2006, Al Viro wrote:

> On Mon, Mar 06, 2006 at 09:56:22AM +0200, Pekka Enberg wrote:
> > On 3/6/06, Dave Jones <davej@redhat.com> wrote:
> > > I wonder if we could get away with something as simple as..
> > >
> > > #define kfree(foo) \
> > >         __kfree(foo); \
> > >         foo = KFREE_POISON;
> > >
> > > ?
> > 
> > It's legal to call kfree() twice for NULL pointer. The above poisons
> > foo unconditionally which makes that case break I think.
> 
> Legal, but rather bad taste.  Init to NULL, possibly assign the value
> if kmalloc(), then kfree() unconditionally - sure, but that... almost
> certainly one hell of a lousy cleanup logics somewhere.
> 
I agree with you.

However, a few months ago it was advocated to let kfree take care of 
testing the pointer against NULL and a load of patches like this:

[PATCH] kfree cleanup: drivers/scsi
author	Jesper Juhl <jesper.juhl@gmail.com>
	Mon, 7 Nov 2005 09:01:26 +0000 (01:01 -0800)
committer	Linus Torvalds <torvalds@g5.osdl.org>
	Mon, 7 Nov 2005 15:54:01 +0000 (07:54 -0800)
commit	c9475cb0c358ff0dd473544280d92482df491913
tree	091617d0bdab9273d44139c86af21b7540e6d9b1	tree
parent	089b1dbbde28f0f641c20beabba28fa89ab4fab9	commit | 
commitdiff
[PATCH] kfree cleanup: drivers/scsi

This is the drivers/scsi/ part of the big kfree cleanup patch.

Remove pointless checks for NULL prior to calling kfree() in 
drivers/scsi/.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Cc: James Bottomley <James.Bottomley@steeleye.com>
Acked-by: Kai Makisara <kai.makisara@kolumbus.fi>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>


went in. I wonder what will come next when wind changes.

-- 
Kai

Re: 9pfs double kfree

[Al Viro]

On Mon, Mar 06, 2006 at 10:40:03AM +0200, Kai Makisara wrote:
> > Legal, but rather bad taste.  Init to NULL, possibly assign the value
> > if kmalloc(), then kfree() unconditionally - sure, but that... almost
> > certainly one hell of a lousy cleanup logics somewhere.
> > 
> I agree with you.
> 
> However, a few months ago it was advocated to let kfree take care of 
> testing the pointer against NULL and a load of patches like this:

That's different - that's _exactly_ the case I've mentioned above.

Moreover, that's exact match to standard behaviour of free(3):

C99 7.20.3.2(2):
The free function causes the space pointed to by ptr to be deallocated, that
is, made available for further allocation.  If ptr is a null pointer, no action
occurs.  Otherwise, if the argument does not match a pointer returned by the
calloc, malloc, or realloc function, or if the space has been deallocated by
a call to free or realloc, the behaviour is undefined.

IOW, free(NULL) is guaranteed to be no-op while double-free is nasal daemon
country.

Re: 9pfs double kfree

[Pavel Machek]

On Po 06-03-06 09:34:01, Al Viro wrote:
> On Mon, Mar 06, 2006 at 10:40:03AM +0200, Kai Makisara wrote:
> > > Legal, but rather bad taste.  Init to NULL, possibly assign the value
> > > if kmalloc(), then kfree() unconditionally - sure, but that... almost
> > > certainly one hell of a lousy cleanup logics somewhere.
> > > 
> > I agree with you.
> > 
> > However, a few months ago it was advocated to let kfree take care of 
> > testing the pointer against NULL and a load of patches like this:
> 
> That's different - that's _exactly_ the case I've mentioned above.
> 
> Moreover, that's exact match to standard behaviour of free(3):
> 
> C99 7.20.3.2(2):
> The free function causes the space pointed to by ptr to be deallocated, that
> is, made available for further allocation.  If ptr is a null pointer, no action
> occurs.  Otherwise, if the argument does not match a pointer returned by the
> calloc, malloc, or realloc function, or if the space has been deallocated by
> a call to free or realloc, the behaviour is undefined.
> 
> IOW, free(NULL) is guaranteed to be no-op while double-free is nasal daemon
> country.

Well, double-free of NULL is permitted by text above. 'If ptr is a
null pointer, no action occurs.'

OTOH #define kfree(a) { __kfree(a); a = NULL; } actually does the
right thing... even with double free.
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Re: 9pfs double kfree

[Andrew Morton]

Dave Jones <davej@redhat.com> wrote:
>
> Probably the first of many found with Coverity.
> 
> This is kfree'd outside of both arms of the if condition already,
> so fall through and free it just once.
> 
> Second variant is double-nasty, it deref's the free'd fcall
> before it tries to free it a second time.
> 
> (I wish we had a kfree variant that NULL'd the target when it was free'd)
> 
> Coverity bugs: 987, 986
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> 
> --- linux-2.6.15.noarch/fs/9p/vfs_super.c~	2006-03-06 01:53:38.000000000 -0500
> +++ linux-2.6.15.noarch/fs/9p/vfs_super.c	2006-03-06 01:54:36.000000000 -0500
> @@ -156,7 +156,6 @@ static struct super_block *v9fs_get_sb(s
>  	stat_result = v9fs_t_stat(v9ses, newfid, &fcall);
>  	if (stat_result < 0) {
>  		dprintk(DEBUG_ERROR, "stat error\n");
> -		kfree(fcall);
>  		v9fs_t_clunk(v9ses, newfid);
>  	} else {
>  		/* Setup the Root Inode */
> --- linux-2.6.15.noarch/fs/9p/vfs_inode.c~	2006-03-06 01:57:05.000000000 -0500
> +++ linux-2.6.15.noarch/fs/9p/vfs_inode.c	2006-03-06 01:58:05.000000000 -0500
> @@ -274,7 +274,6 @@ v9fs_create(struct v9fs_session_info *v9
>  		PRINT_FCALL_ERROR("clone error", fcall);
>  		goto error;
>  	}
> -	kfree(fcall);
>  
>  	err = v9fs_t_create(v9ses, fid, name, perm, mode, &fcall);
>  	if (err < 0) {

The second hunk is probably right and I guess the first one has to be right
as well, but it's all rather obscure, complex and (naturally) comment-free.

So I'll duck on this - Eric, could you please handle it?  Consider doing
away with the dynamically-allocated thing altogether and just allocating it
on the outermost caller's stack.  

Re: 9pfs double kfree

[Eric Van Hensbergen]

On 3/6/06, Andrew Morton <akpm@osdl.org> wrote:
>
> So I'll duck on this - Eric, could you please handle it?  Consider doing
> away with the dynamically-allocated thing altogether and just allocating it
> on the outermost caller's stack.
>

Sure - Lucho may have already caught these, he's got a couple of
patches in the queue and he reported fixing some problems of this
nature that were in our recent patches.

             -eric

Re: 9pfs double kfree

[Latchesar Ionkov]

Hi,

The first issue is a bug, I'll send a patch in a few minutes.

The second one is not a bug. kfree on line 477 is freeing the
strucuture allocated by v9fs_t_walk. v9fs_t_create call overwrites
fcall (after the call it is either NULL or points to another memory
area) so the kfree on line 294 (or line 301 in case of error) don't do
double-free.

There is another bug in the second function though. If v9fs_get_idpool
fails, the execution goes to error label and kfree is called on the
uninitialized fcall. I'll include a fix of this bug too.

Thanks,
    Lucho

On 3/6/06, Dave Jones <davej@redhat.com> wrote:
> Probably the first of many found with Coverity.
>
> This is kfree'd outside of both arms of the if condition already,
> so fall through and free it just once.
>
> Second variant is double-nasty, it deref's the free'd fcall
> before it tries to free it a second time.
>
> (I wish we had a kfree variant that NULL'd the target when it was free'd)
>
> Coverity bugs: 987, 986
>
> Signed-off-by: Dave Jones <davej@redhat.com>
>
>
> --- linux-2.6.15.noarch/fs/9p/vfs_super.c~      2006-03-06 01:53:38.000000000 -0500
> +++ linux-2.6.15.noarch/fs/9p/vfs_super.c       2006-03-06 01:54:36.000000000 -0500
> @@ -156,7 +156,6 @@ static struct super_block *v9fs_get_sb(s
>         stat_result = v9fs_t_stat(v9ses, newfid, &fcall);
>         if (stat_result < 0) {
>                 dprintk(DEBUG_ERROR, "stat error\n");
> -               kfree(fcall);
>                 v9fs_t_clunk(v9ses, newfid);
>         } else {
>                 /* Setup the Root Inode */
> --- linux-2.6.15.noarch/fs/9p/vfs_inode.c~      2006-03-06 01:57:05.000000000 -0500
> +++ linux-2.6.15.noarch/fs/9p/vfs_inode.c       2006-03-06 01:58:05.000000000 -0500
> @@ -274,7 +274,6 @@ v9fs_create(struct v9fs_session_info *v9
>                 PRINT_FCALL_ERROR("clone error", fcall);
>                 goto error;
>         }
> -       kfree(fcall);
>
>         err = v9fs_t_create(v9ses, fid, name, perm, mode, &fcall);
>         if (err < 0) {
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: 9pfs double kfree

[Latchesar Ionkov]

We can't allocate it on the outermost caller's stack. The memory
pointed by v9fs_fcall strucures has variable size and depends on the
incoming 9P messages.

Thanks,
    Lucho

On 3/6/06, Andrew Morton <akpm@osdl.org> wrote:
> Dave Jones <davej@redhat.com> wrote:
> >
> > Probably the first of many found with Coverity.
> >
> > This is kfree'd outside of both arms of the if condition already,
> > so fall through and free it just once.
> >
> > Second variant is double-nasty, it deref's the free'd fcall
> > before it tries to free it a second time.
> >
> > (I wish we had a kfree variant that NULL'd the target when it was free'd)
> >
> > Coverity bugs: 987, 986
> >
> > Signed-off-by: Dave Jones <davej@redhat.com>
> >
> >
> > --- linux-2.6.15.noarch/fs/9p/vfs_super.c~    2006-03-06 01:53:38.000000000 -0500
> > +++ linux-2.6.15.noarch/fs/9p/vfs_super.c     2006-03-06 01:54:36.000000000 -0500
> > @@ -156,7 +156,6 @@ static struct super_block *v9fs_get_sb(s
> >       stat_result = v9fs_t_stat(v9ses, newfid, &fcall);
> >       if (stat_result < 0) {
> >               dprintk(DEBUG_ERROR, "stat error\n");
> > -             kfree(fcall);
> >               v9fs_t_clunk(v9ses, newfid);
> >       } else {
> >               /* Setup the Root Inode */
> > --- linux-2.6.15.noarch/fs/9p/vfs_inode.c~    2006-03-06 01:57:05.000000000 -0500
> > +++ linux-2.6.15.noarch/fs/9p/vfs_inode.c     2006-03-06 01:58:05.000000000 -0500
> > @@ -274,7 +274,6 @@ v9fs_create(struct v9fs_session_info *v9
> >               PRINT_FCALL_ERROR("clone error", fcall);
> >               goto error;
> >       }
> > -     kfree(fcall);
> >
> >       err = v9fs_t_create(v9ses, fid, name, perm, mode, &fcall);
> >       if (err < 0) {
>
> The second hunk is probably right and I guess the first one has to be right
> as well, but it's all rather obscure, complex and (naturally) comment-free.
>
> So I'll duck on this - Eric, could you please handle it?  Consider doing
> away with the dynamically-allocated thing altogether and just allocating it
> on the outermost caller's stack.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH] v9fs: fix for access to unitialized variables or freed memory

[Latchesar Ionkov]

Miscellaneous fixes related to accessing uninitialized variables or memory
that was already freed. Adds function declarations missed in
v9fs-print-9p-messages.patch.

Signed-off-by: Latchesar Ionkov <lucho@ionkov.net>

---
commit f6abafa691503c0a0c8663a1797c867f0f87f13a
tree c3b48d2342dacf62f7b3ce01b72d5a5ec7511481
parent 066de5e57e6c8c8a1e0a6c49a342118f474b21e9
author Latchesar Ionkov <lucho@ionkov.net> Tue, 07 Mar 2006 07:35:04 -0500
committer Latchesar Ionkov <lucho@ionkov.net> Tue, 07 Mar 2006 07:35:04 -0500

 fs/9p/9p.c        |    1 -
 fs/9p/9p.h        |    3 +++
 fs/9p/trans_fd.c  |    1 +
 fs/9p/vfs_inode.c |    8 +++-----
 fs/9p/vfs_super.c |    1 -
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/9p/9p.c b/fs/9p/9p.c
index 1a6d087..f86a28d 100644
--- a/fs/9p/9p.c
+++ b/fs/9p/9p.c
@@ -111,7 +111,6 @@ static void v9fs_t_clunk_cb(void *a, str
 	if (!rc)
 		return;
 
-	dprintk(DEBUG_9P, "tcall id %d rcall id %d\n", tc->id, rc->id);
 	v9ses = a;
 	if (rc->id == RCLUNK)
 		v9fs_put_idpool(fid, &v9ses->fidpool);
diff --git a/fs/9p/9p.h b/fs/9p/9p.h
index 0cd374d..bb8cbac 100644
--- a/fs/9p/9p.h
+++ b/fs/9p/9p.h
@@ -374,3 +374,6 @@ int v9fs_t_read(struct v9fs_session_info
 int v9fs_t_write(struct v9fs_session_info *v9ses, u32 fid, u64 offset,
 		 u32 count, const char __user * data,
 		 struct v9fs_fcall **rcall);
+int v9fs_printfcall(char *, int, struct v9fs_fcall *, int);
+int v9fs_dumpdata(char *, int, u8 *, int);
+int v9fs_printstat(char *, int, struct v9fs_stat *, int);
diff --git a/fs/9p/trans_fd.c b/fs/9p/trans_fd.c
index 1a28ef9..5b2ce21 100644
--- a/fs/9p/trans_fd.c
+++ b/fs/9p/trans_fd.c
@@ -80,6 +80,7 @@ static int v9fs_fd_send(struct v9fs_tran
 	if (!trans || trans->status != Connected || !ts)
 		return -EIO;
 
+	oldfs = get_fs();
 	set_fs(get_ds());
 	/* The cast to a user pointer is valid due to the set_fs() */
 	ret = vfs_write(ts->out_file, (void __user *)v, len, &ts->out_file->f_pos);
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index dce729d..3ad8455 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -265,8 +265,7 @@ v9fs_create(struct v9fs_session_info *v9
 	fid = v9fs_get_idpool(&v9ses->fidpool);
 	if (fid < 0) {
 		eprintk(KERN_WARNING, "no free fids available\n");
-		err = -ENOSPC;
-		goto error;
+		return -ENOSPC;
 	}
 
 	err = v9fs_t_walk(v9ses, pfid, fid, NULL, &fcall);
@@ -313,8 +312,7 @@ v9fs_clone_walk(struct v9fs_session_info
 	nfid = v9fs_get_idpool(&v9ses->fidpool);
 	if (nfid < 0) {
 		eprintk(KERN_WARNING, "no free fids available\n");
-		err = -ENOSPC;
-		goto error;
+		return ERR_PTR(-ENOSPC);
 	}
 
 	err = v9fs_t_walk(v9ses, fid, nfid, (char *) dentry->d_name.name,
@@ -612,7 +610,7 @@ static struct dentry *v9fs_vfs_lookup(st
 	int result = 0;
 
 	dprintk(DEBUG_VFS, "dir: %p dentry: (%s) %p nameidata: %p\n",
-		dir, dentry->d_iname, dentry, nameidata);
+		dir, dentry->d_name.name, dentry, nameidata);
 
 	sb = dir->i_sb;
 	v9ses = v9fs_inode2v9ses(dir);
diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index cdf787e..d05318f 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -156,7 +156,6 @@ static struct super_block *v9fs_get_sb(s
 	stat_result = v9fs_t_stat(v9ses, newfid, &fcall);
 	if (stat_result < 0) {
 		dprintk(DEBUG_ERROR, "stat error\n");
-		kfree(fcall);
 		v9fs_t_clunk(v9ses, newfid);
 	} else {
 		/* Setup the Root Inode */

Re: [PATCH] v9fs: fix for access to unitialized variables or freed memory

[Andrew Morton]

Latchesar Ionkov <lucho@advancedsolutions.com> wrote:
>
> Miscellaneous fixes related to accessing uninitialized variables or memory
> that was already freed.

That part of your patch is applicable to (and needed in) 2.6.16.  (Please
confirm).

> Adds function declarations missed in
> v9fs-print-9p-messages.patch.

And that part is only applicable to a patch which is queued for 2.6.17.

Hence I split your patch into two patches.  The latter one will be folded
into v9fs-print-9p-messages-fix before I send it off to Linus.

The general rule is "one concept per patch", please.  This patch had two.

Re: 9pfs double kfree

[Luke-Jr]

On Monday 06 March 2006 07:56, Pekka Enberg wrote:
> On 3/6/06, Dave Jones <davej@redhat.com> wrote:
> > I wonder if we could get away with something as simple as..
> >
> > #define kfree(foo) \
> >         __kfree(foo); \
> >         foo = KFREE_POISON;
> >
> > ?
>
> It's legal to call kfree() twice for NULL pointer. The above poisons
> foo unconditionally which makes that case break I think.

#define kfree(foo) foo = __kfree(foo);

Assuming the current kfree doesn't return something...