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 <[email protected]>
--- 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) {
From: Dave Jones <[email protected]>
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.
On 3/6/06, David S. Miller <[email protected]> wrote:
> From: Dave Jones <[email protected]>
> 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
On Sun, Mar 05, 2006 at 11:07:11PM -0800, David S. Miller wrote:
> From: Dave Jones <[email protected]>
> 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.
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 <[email protected]>
> > 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
On Mon, Mar 06, 2006 at 12:56:03PM +0530, Balbir Singh wrote:
> On 3/6/06, David S. Miller <[email protected]> wrote:
> > From: Dave Jones <[email protected]>
> > 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
> 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.
On 3/6/06, Dave Jones <[email protected]> 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
On Mon, Mar 06, 2006 at 09:56:22AM +0200, Pekka Enberg wrote:
> On 3/6/06, Dave Jones <[email protected]> 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
On Mon, Mar 06, 2006 at 09:56:22AM +0200, Pekka Enberg wrote:
> On 3/6/06, Dave Jones <[email protected]> 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.
On 3/6/06, Al Viro <[email protected]> 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
On Mon, 2006-03-06 at 10:23 +0200, Pekka Enberg wrote:
> On 3/6/06, Al Viro <[email protected]> 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 ;)
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 <[email protected]> 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 <[email protected]>
Mon, 7 Nov 2005 09:01:26 +0000 (01:01 -0800)
committer Linus Torvalds <[email protected]>
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 <[email protected]>
Cc: James Bottomley <[email protected]>
Acked-by: Kai Makisara <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
went in. I wonder what will come next when wind changes.
--
Kai
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.
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 (http://www.sf.net/projects/suspend) wanted...
Dave Jones <[email protected]> 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 <[email protected]>
>
>
> --- 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.
On 3/6/06, Andrew Morton <[email protected]> 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
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 <[email protected]> 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 <[email protected]>
>
>
> --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
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 <[email protected]> wrote:
> Dave Jones <[email protected]> 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 <[email protected]>
> >
> >
> > --- 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
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 <[email protected]>
---
commit f6abafa691503c0a0c8663a1797c867f0f87f13a
tree c3b48d2342dacf62f7b3ce01b72d5a5ec7511481
parent 066de5e57e6c8c8a1e0a6c49a342118f474b21e9
author Latchesar Ionkov <[email protected]> Tue, 07 Mar 2006 07:35:04 -0500
committer Latchesar Ionkov <[email protected]> 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 */
Latchesar Ionkov <[email protected]> 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.
On Monday 06 March 2006 07:56, Pekka Enberg wrote:
> On 3/6/06, Dave Jones <[email protected]> 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...