[PATCH] include/linux/slab.h: new KFREE() macro.

[PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

Description: new KFREE() macro to set the variable NULL after freeing it.

Signed-off-by: Amit Choudhary <amit2030@gmail.com>

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 1ef822e..28da74c 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -75,6 +75,12 @@ void *__kzalloc(size_t, gfp_t);
 void kfree(const void *);
 unsigned int ksize(const void *);
 
+#define KFREE(x)		\
+	do {			\
+		kfree(x);	\
+		x = NULL;	\
+	} while(0)
+
 /**
  * kcalloc - allocate memory for an array. The memory is set to zero.
  * @n: number of elements.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Segher Boessenkool]

> +#define KFREE(x)		\
> +	do {			\
> +		kfree(x);	\
> +		x = NULL;	\
> +	} while(0)

This doesn't work correctly if "x" has side effects --
double evaluation.  Use a temporary variable instead,
or better, an inline function.  A better name wouldn't
hurt either (kfree_and_zero()?)


Segher

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Pekka Enberg]

On 1/1/07, Amit Choudhary <amit2030@gmail.com> wrote:
> +#define KFREE(x)               \
> +       do {                    \
> +               kfree(x);       \
> +               x = NULL;       \
> +       } while(0)

NAK until you have actual callers for it. CONFIG_SLAB_DEBUG already
catches use after free and double-free so I don't see the point of
this.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Christoph Hellwig]

On Mon, Jan 01, 2007 at 11:23:20PM +0200, Pekka Enberg wrote:
> NAK until you have actual callers for it. CONFIG_SLAB_DEBUG already
> catches use after free and double-free so I don't see the point of
> this.

And CONFIG_SLAB_DEBUG actually finds them for real using poisoning,
unlike setting the pointer to NULL which is too magic to catch most
bugs but rather papers over them.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

>>On 1/1/07, Amit Choudhary <amit2030@xxxxxxxxx> wrote:

>>    +#define KFREE(x) \
>>    + do { \
>>    + kfree(x); \
>>    + x = NULL; \
>>    + } while(0)


>>NAK until you have actual callers for it. CONFIG_SLAB_DEBUG already
>>catches use after free and double-free so I don't see the point of
>>this.

Well, I am not proposing this as a debugging aid. The idea is about correct programming, atleast
from my view. Ideally, if you kfree(x), then you should set x to NULL. So, either programmers do
it themselves or a ready made macro do it for them.

In my opinion, the programmers may welcome the macro that does it for them.

There is another good part about it that results in better programming and shorter code.

Consider an array x[10]. I allocate memory for all of them and then kfree them - kfree(x[0]),
kfree(x[1]), etc. But I do not set these elements to NULL. So,

x[0] = _location_0_already_freed
x[1] = _location_1_already_freed

... and so on...

Now, consider that when I try to allocate memory again, memory allocation fails at x[2]. So, we
have now,

x[0] = _valid_location_0
x[1] = _valid_location_1
x[2] = NULL
x[3] = _location_3_already_freed

So, now to avoid error path leak I have to free all the allocated memory. For this I have to
remember where the allocation failed because I cannot do kfree(x[3]) because it will crash.

You can easily visualize that how KFREE would have helped. Since, I have already KFREE'D them
earlier, x[3] is guaranteed to be NULL and I can free the entire array 'x' wihtout worrying.

So, the code becomes simpler.

Now, consider that there are two more arrays like 'x' being used in the same function. So, now we
have 'x', 'y', 'z' all allocating memory in the same function. Memory allocation can fail at
anytime. So, not only do I have to remember the element location where the allocation failed but
also the array that contains that element.

So, to avoid error path leak, we will have something like this (assume same number of elements in
all arrays):

case z_nomem:
              free_from_0_to_i
              free_all_'y'
              free_all_'x'
              return;

case y_nomem:
              free_from_0_to_i
              free_all_'x'
              return;

case x_nomem:
              free_from_0_to_i
              return;

However, if the programmer would have used KFREE, then the error path leak code have been shorter
and easier.

case nomem:
              free_all_'z'
              free_all_'y'
              free_all_'x'
              return;

I hope that I have made my point but please let me know if I have missed out something or made
some assumption that is not correct.

Regards,
Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Christoph Hellwig]

On Sun, Jan 07, 2007 at 12:46:50AM -0800, Amit Choudhary wrote:
> Well, I am not proposing this as a debugging aid. The idea is about correct programming, atleast
> from my view. Ideally, if you kfree(x), then you should set x to NULL. So, either programmers do
> it themselves or a ready made macro do it for them.

No, you should not.  I suspect that's the basic point you're missing.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jan 07, 2007 at 12:46:50AM -0800, Amit Choudhary wrote:
> > Well, I am not proposing this as a debugging aid. The idea is about correct programming,
> atleast
> > from my view. Ideally, if you kfree(x), then you should set x to NULL. So, either programmers
> do
> > it themselves or a ready made macro do it for them.
> 
> No, you should not.  I suspect that's the basic point you're missing.
> 
> 

Any strong reason why not? x has some value that does not make sense and can create only problems.
And as I explained, it can result in longer code too. So, why keep this value around. Why not
re-initialize it to NULL.

If x should not be re-initialized to NULL, then by the same logic, we should not even initialize
local variables. And all of us know that local variables should be initialized.

I would like to know a good reason as to why x should not be set to NULL.

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Vadim Lobanov]

On Sun, 2007-01-07 at 14:43 -0800, Amit Choudhary wrote:
> Any strong reason why not? x has some value that does not make sense and can create only problems.
> And as I explained, it can result in longer code too. So, why keep this value around. Why not
> re-initialize it to NULL.

Because it looks really STRANGE(tm). Consider the following function,
which is essentially what you're proposing in macro-ized form:
	void foobar(void)
	{
		void *ptr;

		ptr = kmalloc(...);
		// actual work here
		kfree(ptr);
		ptr = NULL;
	}
Reading code like that makes me say "wtf?", simply because 'ptr' is not
used thereafter, so setting it to NULL is both pointless and confusing
(it looks out-of-place, and therefore makes me wonder if there's
something stupidly tricky going on).

Also, arguably, your demonstration of why the lack of the proposed
KFREE() macro results in longer code is invalid. Whereas you wrote:
	pointer *arr_x[size_x];
	pointer *arr_y[size_y];
	pointer *arr_z[size_z];
That really should have been:
	pointer *arr[size_x + size_y + size_z];
or:
	pointer **arr[3] = { arr_x, arr_y, arr_z };
In which case, the you only need one path in the function to handle
allocation failures, rather than the three that you were arguing for.

> If x should not be re-initialized to NULL, then by the same logic, we should not even initialize
> local variables. And all of us know that local variables should be initialized.

That's some strange and confused logic then. Here's my stab at the same
logical premise: "Using uninitialized values is bad." Notice how that,
in and of itself, makes no statements regarding freed pointers, since
the intent is not to use them after they've been freed anyway.

-- Vadim Lobanov

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> On Sun, 2007-01-07 at 14:43 -0800, Amit Choudhary wrote:
> > Any strong reason why not? x has some value that does not make sense and can create only
> problems.
> > And as I explained, it can result in longer code too. So, why keep this value around. Why not
> > re-initialize it to NULL.
> 
> Because it looks really STRANGE(tm). Consider the following function,
> which is essentially what you're proposing in macro-ized form:
> 	void foobar(void)
> 	{
> 		void *ptr;
> 
> 		ptr = kmalloc(...);
> 		// actual work here
> 		kfree(ptr);
> 		ptr = NULL;
> 	}

That's where KFREE(ptr) comes in so that the code doesn't look ugly and still the purpose is
achieved.

"I still do not know of a single good reason as to why we should not do this."

And if all programmers did the right thing always then why do we have all the debugging options in
the first place.

> Reading code like that makes me say "wtf?", simply because 'ptr' is not
> used thereafter,

Really? Then why do we have all the debugging options to catch re-use of the memory that has been
freed. So many debugging options has been implemented, so much effort has gone into them, partly
because programmers sometimes miss correct programming.

> so setting it to NULL is both pointless and confusing
> (it looks out-of-place, and therefore makes me wonder if there's
> something stupidly tricky going on).
> 
> Also, arguably, your demonstration of why the lack of the proposed
> KFREE() macro results in longer code is invalid. Whereas you wrote:
> 	pointer *arr_x[size_x];
> 	pointer *arr_y[size_y];
> 	pointer *arr_z[size_z];
> That really should have been:
> 	pointer *arr[size_x + size_y + size_z];
> or:
> 	pointer **arr[3] = { arr_x, arr_y, arr_z };
> In which case, the you only need one path in the function to handle
> allocation failures, rather than the three that you were arguing for.
> 

I do not know what you are talking about here. You are saying that a function does not need three
different arrays with different names. How can you say that? How do you know what is the
requirement?

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Vadim Lobanov]

On Sun, 2007-01-07 at 16:02 -0800, Amit Choudhary wrote:
> That's where KFREE(ptr) comes in so that the code doesn't look ugly and still the purpose is
> achieved.

Shoving it into a macro makes it no better.

> > Reading code like that makes me say "wtf?", simply because 'ptr' is not
> > used thereafter,
> 
> Really? Then why do we have all the debugging options to catch re-use of the memory that has been
> freed. So many debugging options has been implemented, so much effort has gone into them, partly
> because programmers sometimes miss correct programming.

Which is exactly why we should continue to let programmers focus on
trying to get their code correct and letting the existing safety tools
catch thinkos, instead of distracting them with confusing and completely
pointless variable assignments.

> I do not know what you are talking about here. You are saying that a function does not need three
> different arrays with different names. How can you say that? How do you know what is the
> requirement?

What I said was that your example proves something entirely different
than what you think: rather than proving the need for your KFREE()
macro, it instead proves the need to design the code correctly from the
start, so as to avoid even thinking about this crud.

If you insist, here's your example function, trivially rewritten without
any NULL assignments. (I used two arrays, not three, to save space. The
basic idea works by-design for any random number of arrays, each of any
arbitrary size.)

struct type1 {
	/* something */
};

struct type2 {
	/* something */
};

#define COUNT 10

void function1(struct type1 **a1, struct type2 **a2, unsigned int sz);

void function2(void)
{
	struct type1 *arr1[COUNT];
	struct type2 *arr2[COUNT];
	int i;

	/* init */
	for (i = 0; i < COUNT; i++) {
		arr1[i] = kmalloc(sizeof(struct type1), ...);
		if (!arr1[i])
			goto free_1;
	}
	for (i = 0; i < COUNT; i++) {
		arr2[i] = kmalloc(sizeof(struct type2), ...);
		if (!arr2[i])
			goto free_2;
	}

	/* work */
	function1(arr1, arr2, COUNT);

	/* fini */
	i = COUNT;
free_2:
	for (i--; i >= 0; i--) {
		kfree(arr2[i]);
	}
	i = COUNT;
free_1:
	for (i--; i >= 0; i--) {
		kfree(arr1[i]);
	}
}

In most cases, though, the above code design would be brain-damaged from
the start: unless the sizes involved are prohibitively large, the
function should be allocating all the memory in a single pass.

So, where's the demonstrated need for KFREE()?

-- Vadim Lobanov

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> 
> struct type1 {
> 	/* something */
> };
> 
> struct type2 {
> 	/* something */
> };
> 
> #define COUNT 10
> 
> void function1(struct type1 **a1, struct type2 **a2, unsigned int sz);
> 
> void function2(void)
> {
> 	struct type1 *arr1[COUNT];
> 	struct type2 *arr2[COUNT];
> 	int i;
> 
> 	/* init */
> 	for (i = 0; i < COUNT; i++) {
> 		arr1[i] = kmalloc(sizeof(struct type1), ...);
> 		if (!arr1[i])
> 			goto free_1;
> 	}
> 	for (i = 0; i < COUNT; i++) {
> 		arr2[i] = kmalloc(sizeof(struct type2), ...);
> 		if (!arr2[i])
> 			goto free_2;
> 	}
> 
> 	/* work */
> 	function1(arr1, arr2, COUNT);
> 
> 	/* fini */
> 	i = COUNT;
> free_2:
> 	for (i--; i >= 0; i--) {
> 		kfree(arr2[i]);
> 	}
> 	i = COUNT;
> free_1:
> 	for (i--; i >= 0; i--) {
> 		kfree(arr1[i]);
> 	}
> }
> 
> In most cases, though, the above code design would be brain-damaged from
> the start: unless the sizes involved are prohibitively large, the
> function should be allocating all the memory in a single pass.
> 
> So, where's the demonstrated need for KFREE()?

I have already explained it earlier. I will try again. You will not need free_2: and free_1: with
KFREE(). You will only need one free: with KFREE.

Also, let's say that count is different for each array? Then how do you propose that memory be
allocated in one pass?

>In most cases, though, the above code design would be brain-damaged from
>the start: unless the sizes involved are prohibitively large, the
>function should be allocating all the memory in a single pass.

Well, only if everything would be fine and correct, we would not be needing anything. If you think
this kind of code is brain-damaged then the linux kernel has couple of these.

I have scanned the whole kernel to check whether people are checking for return values of kmalloc,
I found that at many places they don't and have sent patches for them. Now, this too is brain
damaged code. And during the scan I saw examples of what I described earlier.

KFREE() can fix some of those cases.

Consider this as the proof of what I explained earlier. This fucntion was broken but I fixed it
and then realized why KFREE() is needed. 2 kmallocs and 1 usb_alloc_urb(). Well, I can only give
examples why KFREE() is needed. If you do not agree, I cannot force you to agree with me. And if
you really do not want to agree then even my examples will fail. Also, if you think that people
are not doing KFREE() kind of stuff then you should scan the kernel and you will see it for
yourself.


Below are some examples where people are doing KFREE() kind of stuff:

--
arch/ppc/kernel/smp-tbsync.c:	kfree( tbsync );
arch/ppc/kernel/smp-tbsync.c-	tbsync = NULL;
--
arch/ppc/8260_io/fcc_enet.c:			dev_kfree_skb(fep->tx_skbuff[i]);
arch/ppc/8260_io/fcc_enet.c-			fep->tx_skbuff[i] = NULL;
--
arch/ppc/8xx_io/cs4218_tdm.c:			kfree (sound_buffers);
arch/ppc/8xx_io/cs4218_tdm.c-			sound_buffers = 0;
--
arch/ia64/kernel/topology.c:	kfree(all_cpu_cache_info[cpu].cache_leaves);
arch/ia64/kernel/topology.c-	all_cpu_cache_info[cpu].cache_leaves = NULL;
--
arch/ia64/kernel/acpi.c:	kfree(buffer.pointer);
arch/ia64/kernel/acpi.c-	buffer.pointer = NULL;
--
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c:				kfree(acpi_perf_data[j]);
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c-				acpi_perf_data[j] = NULL;
--

There are many more and you can scan the kernel yourself.

Below is where memory is allocated in different arrays with different counts:

static int stv680_start_stream (struct usb_stv *stv680)
{
	struct urb *urb;
	int err = 0, i;

	stv680->streaming = 1;

	/* Do some memory allocation */
	for (i = 0; i < STV680_NUMFRAMES; i++) {
		stv680->frame[i].data = stv680->fbuf + i * stv680->maxframesize;
		stv680->frame[i].curpix = 0;
	}
	/* packet size = 4096  */
	for (i = 0; i < STV680_NUMSBUF; i++) {
		stv680->sbuf[i].data = kmalloc (stv680->rawbufsize, GFP_KERNEL);
		if (stv680->sbuf[i].data == NULL) {
			PDEBUG (0, "STV(e): Could not kmalloc raw data buffer %i", i);
			goto nomem_err;
		}
	}

	stv680->scratch_next = 0;
	stv680->scratch_use = 0;
	stv680->scratch_overflow = 0;
	for (i = 0; i < STV680_NUMSCRATCH; i++) {
		stv680->scratch[i].data = kmalloc (stv680->rawbufsize, GFP_KERNEL);
		if (stv680->scratch[i].data == NULL) {
			PDEBUG (0, "STV(e): Could not kmalloc raw scratch buffer %i", i);
			goto nomem_err;
		}
		stv680->scratch[i].state = BUFFER_UNUSED;
	}

	for (i = 0; i < STV680_NUMSBUF; i++) {
		urb = usb_alloc_urb (0, GFP_KERNEL);
		if (!urb)
			goto nomem_err;

		/* sbuf is urb->transfer_buffer, later gets memcpyed to scratch */
		usb_fill_bulk_urb (urb, stv680->udev,
				   usb_rcvbulkpipe (stv680->udev, stv680->bulk_in_endpointAddr),
				   stv680->sbuf[i].data, stv680->rawbufsize,
				   stv680_video_irq, stv680);
		stv680->urb[i] = urb;
		err = usb_submit_urb (stv680->urb[i], GFP_KERNEL);
		if (err)
			PDEBUG (0, "STV(e): urb burned down in start stream");
	}			/* i STV680_NUMSBUF */

	stv680->framecount = 0;
	return 0;

 nomem_err:
	for (i = 0; i < STV680_NUMSCRATCH; i++) {
		kfree(stv680->scratch[i].data);
		stv680->scratch[i].data = NULL;
	}
	for (i = 0; i < STV680_NUMSBUF; i++) {
		usb_kill_urb(stv680->urb[i]);
		usb_free_urb(stv680->urb[i]);
		stv680->urb[i] = NULL;
		kfree(stv680->sbuf[i].data);
		stv680->sbuf[i].data = NULL;
	}
	return -ENOMEM;

}

static int stv680_stop_stream (struct usb_stv *stv680)
{
	int i;

	if (!stv680->streaming || !stv680->udev)
		return 1;

	stv680->streaming = 0;

	for (i = 0; i < STV680_NUMSBUF; i++)
		if (stv680->urb[i]) {
			usb_kill_urb (stv680->urb[i]);
			usb_free_urb (stv680->urb[i]);
			stv680->urb[i] = NULL;
			kfree(stv680->sbuf[i].data);
		}
	for (i = 0; i < STV680_NUMSCRATCH; i++) {
		kfree(stv680->scratch[i].data);
		stv680->scratch[i].data = NULL;
	}

	return 0;
}

-Amit

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Vadim Lobanov]

On Sun, 2007-01-07 at 20:09 -0800, Amit Choudhary wrote:
> I have already explained it earlier. I will try again. You will not need free_2: and free_1: with
> KFREE(). You will only need one free: with KFREE.

So, to rephrase, your stated goal is to get rid of any non-singular goto
labels in function error handling paths? Aside from sounding trippy in a
non-conformist kind of way, what benefits will this give to the kernel?

I ask this because there's already one easy-to-spot downside: you'll end
up calling kfree(NULL) a bunch of times that can be, and should be,
avoided. Whereas turning my computer into a better space-heater using
noops (like repeated kfree(NULL) calls) may be a noble goal, I'd much
rather not waste this planet's limited resources unnecessarily.

> Also, let's say that count is different for each array? Then how do you propose that memory be
> allocated in one pass?

The parameters to a '+' operator need not be equivalent.

> I have scanned the whole kernel to check whether people are checking for return values of kmalloc,
> I found that at many places they don't and have sent patches for them. Now, this too is brain
> damaged code. And during the scan I saw examples of what I described earlier.

These cases should be fixed independently of any particular KFREE()
agenda.

> KFREE() can fix some of those cases.

I am curious as to how a KFREE() macro can fix cases where people don't
check the kmalloc() return value.

> Below are some examples where people are doing KFREE() kind of stuff:

I glanced at one instance, and...

> arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c:				kfree(acpi_perf_data[j]);
> arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c-				acpi_perf_data[j] = NULL;

'acpi_perf_data' is a global and persistent data structure, where a NULL
value actually has a specific and distinct meaning (as in
acpi_cpufreq_cpu_init()). How you think this helps your argument with
setting a local pointer to NULL after free is beyond me.

-- Vadim Lobanov

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> On Sun, 2007-01-07 at 20:09 -0800, Amit Choudhary wrote:
> > I have already explained it earlier. I will try again. You will not need free_2: and free_1:
> with
> > KFREE(). You will only need one free: with KFREE.
> 

I do not want to write this but I think that you are arguing just for the heck of it. Please be
sane.

-Amit

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Vadim Lobanov]

On Sun, 2007-01-07 at 23:29 -0800, Amit Choudhary wrote:
> I do not want to write this but I think that you are arguing just for the heck of it. Please be
> sane.

No, I'm merely trying to demonstrate, on a logical basis, why the
proposed patch does not (in my opinion) belong within the kernel. The
fact that I'm not alone in voicing such disagreement should mean
something.

-- Vadim Lobanov

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Vadim Lobanov <vlobanov@speakeasy.net> wrote:

> On Sun, 2007-01-07 at 23:29 -0800, Amit Choudhary wrote:
> > I do not want to write this but I think that you are arguing just for the heck of it. Please
> be
> > sane.
> 
> No, I'm merely trying to demonstrate, on a logical basis, why the
> proposed patch does not (in my opinion) belong within the kernel. The
> fact that I'm not alone in voicing such disagreement should mean
> something.
> 

I agree that since couple of people are voicing disagreement the definitely it means something and
probably it means that you are right.

Let's try to apply the same logic to my explanation:

KFREE() macro has __actually__ been used at atleast 1000 places in the kernel by atleast 50
different people. Doesn't that lend enough credibility to what I am saying.

People did something like this 1000 times: kfree(x), x = NULL. I simply proposed the KFREE() macro
that does the same thing. Resistance to something that is already being done in the kernel. I
really do not care whether it goes in the kernel or not. There are lots of other places where I
can contribute. But I do not understand the resistance.

It is already being done in the kernel.

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Al Viro]

On Mon, Jan 08, 2007 at 12:47:07AM -0800, Amit Choudhary wrote:
 
> Let's try to apply the same logic to my explanation:
> 
> KFREE() macro has __actually__ been used at atleast 1000 places in the kernel by atleast 50
> different people. Doesn't that lend enough credibility to what I am saying.

No.  Simple "it happens a lot of times" is _not_ enough to establish
credibility of "it should be done that way".  It is a good reason to
research the rationale in each case.
 
> People did something like this 1000 times: kfree(x), x = NULL. I simply proposed the KFREE() macro
> that does the same thing. Resistance to something that is already being done in the kernel. I
> really do not care whether it goes in the kernel or not. There are lots of other places where I
> can contribute. But I do not understand the resistance.
> 
> It is already being done in the kernel.

And each instance either has a reason for doing it that way or is useless
or is a bug.  Reasons, where they actually exist, very likely are not
uniform.

Blind copying of patterns without understanding what and why they are
doing is a Very Bad Thing(tm).  That's how the bugs are created and
propagated, BTW.

RE: [PATCH] include/linux/slab.h: new KFREE() macro.

[Hua Zhong]

> Any strong reason why not? x has some value that does not 
> make sense and can create only problems.

By the same logic, you should memset the buffer to zero before freeing it too.

> And as I explained, it can result in longer code too. So, why 
> keep this value around. Why not re-initialize it to NULL.

Because initialization increases code size.

It's a silly patch.

> If x should not be re-initialized to NULL, then by the same 
> logic, we should not even initialize local variables. And all 
> of us know that local variables should be initialized.
> 
> I would like to know a good reason as to why x should not be 
> set to NULL.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Pekka Enberg]

On 1/8/07, Hua Zhong <hzhong@gmail.com> wrote:
> > And as I explained, it can result in longer code too. So, why
> > keep this value around. Why not re-initialize it to NULL.
>
> Because initialization increases code size.

And it also effectively blocks the slab debugging code from doing its
job detecting double-frees.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On 1/8/07, Hua Zhong <hzhong@gmail.com> wrote:
> > > And as I explained, it can result in longer code too. So, why
> > > keep this value around. Why not re-initialize it to NULL.
> >
> > Because initialization increases code size.
> 
> And it also effectively blocks the slab debugging code from doing its
> job detecting double-frees.
> 

Man, so you do want someone to set 'x' to NULL after freeing it, so that the slab debugging code
can catch double frees. If you set it to NULL then double free is harmless. So, you want something
harmful in the system and then debug it with the slab debugging code. Man, doesn't make sense to
me.

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Al Viro]

On Mon, Jan 08, 2007 at 12:31:44AM -0800, Amit Choudhary wrote:
> 
> --- Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> 
> > On 1/8/07, Hua Zhong <hzhong@gmail.com> wrote:
> > > > And as I explained, it can result in longer code too. So, why
> > > > keep this value around. Why not re-initialize it to NULL.
> > >
> > > Because initialization increases code size.
> > 
> > And it also effectively blocks the slab debugging code from doing its
> > job detecting double-frees.
> > 
> 
> Man, so you do want someone to set 'x' to NULL after freeing it, so that the slab debugging code
> can catch double frees. If you set it to NULL then double free is harmless. So, you want something
> harmful in the system and then debug it with the slab debugging code. Man, doesn't make sense to
> me.

_Definitely_ cargo-cult programming...

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Sumit Narayan]

Asking for KFREE is as silly as asking for a macro to check if kmalloc
succeeded for a pointer, else return ENOMEM.

#define CKMALLOC(p,x) \
   do {   \
       p = kmalloc(x, GFP_KERNEL); \
       if(!p) return -ENOMEM; \
    } while(0)


On 1/8/07, Amit Choudhary <amit2030@yahoo.com> wrote:
>
> --- Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> > On 1/8/07, Hua Zhong <hzhong@gmail.com> wrote:
> > > > And as I explained, it can result in longer code too. So, why
> > > > keep this value around. Why not re-initialize it to NULL.
> > >
> > > Because initialization increases code size.
> >
> > And it also effectively blocks the slab debugging code from doing its
> > job detecting double-frees.
> >
>
> Man, so you do want someone to set 'x' to NULL after freeing it, so that the slab debugging code
> can catch double frees. If you set it to NULL then double free is harmless. So, you want something
> harmful in the system and then debug it with the slab debugging code. Man, doesn't make sense to
> me.
>
> -Amit
>
>
> __________________________________________________
> Do You Yahoo!?
> Tired of spam?  Yahoo! Mail has the best spam protection around
> http://mail.yahoo.com
> -
> 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: [PATCH] include/linux/slab.h: new KFREE() macro.

[Robert P. J. Day]

On Mon, 8 Jan 2007, Sumit Narayan wrote:

> Asking for KFREE is as silly as asking for a macro to check if
> kmalloc succeeded for a pointer, else return ENOMEM.
>
> #define CKMALLOC(p,x) \
>   do {   \
>       p = kmalloc(x, GFP_KERNEL); \
>       if(!p) return -ENOMEM; \
>    } while(0)

oooooooh ... cool.  i'll whip up a patch for that right away.

rday

p.s.  i'm *kidding*, for god's sake.  um ... what are you doing?  and
where did you get that tire iron?  no, wait ...

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Sumit Narayan <talk2sumit@gmail.com> wrote:

> Asking for KFREE is as silly as asking for a macro to check if kmalloc
> succeeded for a pointer, else return ENOMEM.
> 
> #define CKMALLOC(p,x) \
>    do {   \
>        p = kmalloc(x, GFP_KERNEL); \
>        if(!p) return -ENOMEM; \
>     } while(0)
> 

There are bugs with this approach. This introduces error path leaks. If you have allocated some
memory earlier, then you got to free them.

-Amit

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Pekka Enberg]

Hi Amit,

On 1/8/07, Amit Choudhary <amit2030@yahoo.com> wrote:
> Man, doesn't make sense to me.

Well, man, double-free is a programming error and papering over it
with NULL initializations bloats the kernel and makes the code
confusing.

Clear enough for you?

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> Hi Amit,
> 
> On 1/8/07, Amit Choudhary <amit2030@yahoo.com> wrote:
> > Man, doesn't make sense to me.
> 
> Well, man, double-free is a programming error and papering over it
> with NULL initializations bloats the kernel and makes the code
> confusing.
> 
> Clear enough for you?
> 

It is a programming error because the underlying code cannot handle it. If, from the beginning of
time, double free would have been handled properly then we wouldn't have thought twice about it.

You want to catch double frees. What if double frees are no-ops?

I do not see how a double free can result in _logical_wrong_behaviour_ of the program and the
program keeps on running (like an incoming packet being dropped because of double free). Double
free will _only_and_only_ result in system crash that can be solved by setting 'x' to NULL.

-Amit



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Pekka Enberg]

On 1/8/07, Amit Choudhary <amit2030@yahoo.com> wrote:
> It is a programming error because the underlying code cannot handle it.

Yes. Do you also grasp the fact that there is no way for the allocator
to handle it either? So, double-free, from allocator standpoint can
_never_ be no-op.

What you're proposing is _not_ making double-free no-op, but instead,
making sure we never have a reference to p after kfree(p). However,
(1) that bloats the kernel text and (2) doesn't actually guarantee
that there are _no_ references to p (you can have an alias q for it,
but there's no way for us to know that).

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Mon, 08 Jan 2007 01:06:12 PST, Amit Choudhary said:
> I do not see how a double free can result in _logical_wrong_behaviour_ of the program and the
> program keeps on running (like an incoming packet being dropped because of double free). Double
> free will _only_and_only_ result in system crash that can be solved by setting 'x' to NULL.

The problem is that very rarely is there a second free() with no intervening
use - what actually *happens* usually is:

1) You alloc the memory
2) You use the memory
3) You take a reference on the memory, so you know where it is.
4) You free the memory
5) You use the memory via the reference you took in (3)
6) You free it again - at which point you finally know for sure that
everything in step 5 was doing a fandango on core....

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Valdis.Kletnieks@vt.edu wrote:

> On Mon, 08 Jan 2007 01:06:12 PST, Amit Choudhary said:
> > I do not see how a double free can result in _logical_wrong_behaviour_ of the program and the
> > program keeps on running (like an incoming packet being dropped because of double free).
> Double
> > free will _only_and_only_ result in system crash that can be solved by setting 'x' to NULL.
> 
> The problem is that very rarely is there a second free() with no intervening
> use - what actually *happens* usually is:
> 
> 1) You alloc the memory
> 2) You use the memory
> 3) You take a reference on the memory, so you know where it is.
> 4) You free the memory
> 5) You use the memory via the reference you took in (3)
> 6) You free it again - at which point you finally know for sure that
> everything in step 5 was doing a fandango on core....
> 

Correct. And doing kfree(x); x=NULL; is not hiding that. These issues can still be debugged by
using the slab debugging options. One other benefit of doing this is that if someone tries to
access the same memory again using the variable 'x', then he will get an immediate crash. And the
problem can be solved immediately, without using the slab debugging options. I do not yet
understand how doing this hides the bugs, obfuscates the code, etc. because I haven't seen an
example yet, but only blanket statements.

But now I know better, since I haven't heard anything in support of this case, I have concluded
that doing kfree(x); x=NULL; is _not_needed_ in the "linux kernel". I hope that no one does it in
the future. And since people vehemently opposed this, I think its better to add another item on
the kernel janitor's list to remove all the (x=NULL) statements where people are doing "kfree(x);
x=NULL".

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Randy Dunlap]

On Tue, 9 Jan 2007 11:02:35 -0800 (PST) Amit Choudhary wrote:

> 
> --- Valdis.Kletnieks@vt.edu wrote:
> 
> > On Mon, 08 Jan 2007 01:06:12 PST, Amit Choudhary said:
> > > I do not see how a double free can result in _logical_wrong_behaviour_ of the program and the
> > > program keeps on running (like an incoming packet being dropped because of double free).
> > Double
> > > free will _only_and_only_ result in system crash that can be solved by setting 'x' to NULL.
> > 
> > The problem is that very rarely is there a second free() with no intervening
> > use - what actually *happens* usually is:
> > 
> > 1) You alloc the memory
> > 2) You use the memory
> > 3) You take a reference on the memory, so you know where it is.
> > 4) You free the memory
> > 5) You use the memory via the reference you took in (3)
> > 6) You free it again - at which point you finally know for sure that
> > everything in step 5 was doing a fandango on core....
> > 
> 
> Correct. And doing kfree(x); x=NULL; is not hiding that. These issues can still be debugged by
> using the slab debugging options. One other benefit of doing this is that if someone tries to
> access the same memory again using the variable 'x', then he will get an immediate crash. And the
> problem can be solved immediately, without using the slab debugging options. I do not yet
> understand how doing this hides the bugs, obfuscates the code, etc. because I haven't seen an
> example yet, but only blanket statements.
> 
> But now I know better, since I haven't heard anything in support of this case, I have concluded
> that doing kfree(x); x=NULL; is _not_needed_ in the "linux kernel". I hope that no one does it in
> the future. And since people vehemently opposed this, I think its better to add another item on
> the kernel janitor's list to remove all the (x=NULL) statements where people are doing "kfree(x);
> x=NULL".

No thanks.  If a driver author wants to maintain driver state
that way, it's OK, but that doesn't make it a global requirement.

---
~Randy

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Randy Dunlap <randy.dunlap@oracle.com> wrote:

> On Tue, 9 Jan 2007 11:02:35 -0800 (PST) Amit Choudhary wrote:
> 
> No thanks.  If a driver author wants to maintain driver state
> that way, it's OK, but that doesn't make it a global requirement.
> 

Ok. So, a driver can have its own local definition of KFREE() macro.

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Tue, 09 Jan 2007 11:02:35 PST, Amit Choudhary said:
> Correct. And doing kfree(x); x=NULL; is not hiding that. These issues can still be debugged by
> using the slab debugging options. One other benefit of doing this is that if someone tries to
> access the same memory again using the variable 'x', then he will get an immediate crash. And the
> problem can be solved immediately, without using the slab debugging options. I do not yet
> understand how doing this hides the bugs, obfuscates the code, etc. because I haven't seen an
> example yet, but only blanket statements.

char *broken() {
	char *x, *y;
	x = kmalloc(100);
	y = x;
	kfree(x);
	x = NULL;
	return y;
}

Setting x to NULL doesn't do anything to fix the *real* bug here, because
the problematic reference is held in y, not x.  So you never get a crash
because somebody dereferences x.


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Valdis.Kletnieks@vt.edu wrote:

> On Tue, 09 Jan 2007 11:02:35 PST, Amit Choudhary said:
> > Correct. And doing kfree(x); x=NULL; is not hiding that. These issues can still be debugged by
> > using the slab debugging options. One other benefit of doing this is that if someone tries to
> > access the same memory again using the variable 'x', then he will get an immediate crash.


What did you understand when I wrote that "if you access the same memory again using the variable
'x"?

Using variable 'x' means using variable 'x' and not variable 'y'.


 And
> the
> > problem can be solved immediately, without using the slab debugging options. I do not yet
> > understand how doing this hides the bugs, obfuscates the code, etc. because I haven't seen an
> > example yet, but only blanket statements.
> 
> char *broken() {
> 	char *x, *y;
> 	x = kmalloc(100);
> 	y = x;
> 	kfree(x);
> 	x = NULL;
> 	return y;
> }
> 
> Setting x to NULL doesn't do anything to fix the *real* bug here, because
> the problematic reference is held in y, not x.  


Did I ever say that it fixes that kind of bug?


>So you never get a crash
> because somebody dereferences x.
> 


Dereferencing 'x' means dereferencing 'x' and not dereferencing 'y'.

-Amit

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On Tue, 09 Jan 2007 16:00:51 PST, Amit Choudhary said:

> What did you understand when I wrote that "if you access the same memory again using the variable
> 'x"?
> 
> Using variable 'x' means using variable 'x' and not variable 'y'.

Right - but in real-world code, 'y' is the actual problem.

> Did I ever say that it fixes that kind of bug?

The point you're missing is that you're "fixing" a failure mode that in
general isn't seen, and ignoring the failure mode that *is* seen.

> Dereferencing 'x' means dereferencing 'x' and not dereferencing 'y'.

And neither "x" nor "y" is "the set of all still-live pointers to already-freed
memory" - which is why your proposal doesn't actually do anything effective.


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Jesper Juhl]

On 08/01/07, Amit Choudhary <amit2030@yahoo.com> wrote:
>
> --- Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> > On 1/8/07, Hua Zhong <hzhong@gmail.com> wrote:
> > > > And as I explained, it can result in longer code too. So, why
> > > > keep this value around. Why not re-initialize it to NULL.
> > >
> > > Because initialization increases code size.
> >
> > And it also effectively blocks the slab debugging code from doing its
> > job detecting double-frees.
> >
>
> Man, so you do want someone to set 'x' to NULL after freeing it, so that the slab debugging
> code can catch double frees. If you set it to NULL then double free is harmless.

No, setting the pointer to NULL doesn't make a double free harmless it
just hides a bug. The real fix would be to remove the double free.

If you just set the pointer to NULL and ignore the double free then
you've just bloated the kernel with an extra pointless assignment and
left a kfree() call in the kernel that should not be there.  In my
book that's a bug.
If instead you rework the code to avoid the double free, then you
avoid the pointless NULL assignment and you get rid of a kfree call,
and you also get to review the logic and find the flaw that lead to a
double free in the first place.  A double free is not something we
should just sweep under the carpet and forget about, it's very likely
an indication that some logic is flawed and should be fixed.

This KFREE macro does not belong in the kernel IMHO.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

RE: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

--- Hua Zhong <hzhong@gmail.com> wrote:

> > Any strong reason why not? x has some value that does not 
> > make sense and can create only problems.
> 
> By the same logic, you should memset the buffer to zero before freeing it too.
> 

How does this help?

> > And as I explained, it can result in longer code too. So, why 
> > keep this value around. Why not re-initialize it to NULL.
> 
> Because initialization increases code size.

Then why use kzalloc()? Let's remove _ALL_ the initialization code from the kernel.

Attached is some code from the kernel. Expanded KFREE() has been used atleast 1000 times in the
kernel. By your logic, everyone is stupid in doing so. Something has been done atleast 1000 times
in the kernel, that looks okay. But consolidating it at one place does not look okay. I am listing
some of the 1000 places where KFREE() has been used. All this code have been written by atleast 50
different people. From your logic they were doing "silly" things.

--
arch/ppc/kernel/smp-tbsync.c:	kfree( tbsync );
arch/ppc/kernel/smp-tbsync.c-	tbsync = NULL;
--
arch/powerpc/kernel/smp-tbsync.c:	kfree(tbsync);
arch/powerpc/kernel/smp-tbsync.c-	tbsync = NULL;
--
arch/powerpc/kernel/rtas_flash.c:		kfree(dp->data);
arch/powerpc/kernel/rtas_flash.c-		dp->data = NULL;
--
arch/powerpc/platforms/ps3/spu.c:	kfree(spu->pdata);
arch/powerpc/platforms/ps3/spu.c-	spu->pdata = NULL;
--
arch/powerpc/platforms/cell/spu_priv1_mmio.c:	kfree(spu->pdata);
arch/powerpc/platforms/cell/spu_priv1_mmio.c-	spu->pdata = NULL;
--
arch/powerpc/platforms/cell/spu_priv1_mmio.c:	kfree(spu->pdata);
arch/powerpc/platforms/cell/spu_priv1_mmio.c-	spu->pdata = NULL;
--
arch/powerpc/platforms/cell/spufs/context.c:	kfree(ctx);
arch/powerpc/platforms/cell/spufs/context.c-	ctx = NULL;
--
arch/ia64/kernel/topology.c:	kfree(all_cpu_cache_info[cpu].cache_leaves);
arch/ia64/kernel/topology.c-	all_cpu_cache_info[cpu].cache_leaves = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-		part->channels = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-		part->channels = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-		part->channels = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-		part->channels = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-		part->channels = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:			kfree(ch->local_msgqueue_base);
arch/ia64/sn/kernel/xpc_channel.c-			ch->local_msgqueue = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(ch->notify_queue);
arch/ia64/sn/kernel/xpc_channel.c-		ch->notify_queue = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:		kfree(ch->notify_queue);
arch/ia64/sn/kernel/xpc_channel.c-		ch->notify_queue = NULL;
--
arch/ia64/sn/kernel/xpc_channel.c:	kfree(part->channels);
arch/ia64/sn/kernel/xpc_channel.c-	part->channels = NULL;
--
arch/s390/hypfs/inode.c:		kfree(sb->s_fs_info);
arch/s390/hypfs/inode.c-		sb->s_fs_info = NULL;
--
arch/s390/kernel/debug.c:	kfree(db_info->areas);
arch/s390/kernel/debug.c-	db_info->areas = NULL;
--
arch/sparc64/kernel/of_device.c:		kfree(op);
arch/sparc64/kernel/of_device.c-		op = NULL;
--
arch/sparc64/kernel/us2e_cpufreq.c:		kfree(us2e_freq_table);
arch/sparc64/kernel/us2e_cpufreq.c-		us2e_freq_table = NULL;
--
arch/sparc64/kernel/us2e_cpufreq.c:		kfree(cpufreq_us2e_driver);
arch/sparc64/kernel/us2e_cpufreq.c-		cpufreq_us2e_driver = NULL;
--
arch/sparc64/kernel/us2e_cpufreq.c:		kfree(us2e_freq_table);
arch/sparc64/kernel/us2e_cpufreq.c-		us2e_freq_table = NULL;
--
arch/sparc64/kernel/us3_cpufreq.c:		kfree(us3_freq_table);
arch/sparc64/kernel/us3_cpufreq.c-		us3_freq_table = NULL;
--
arch/sparc64/kernel/us3_cpufreq.c:		kfree(cpufreq_us3_driver);
arch/sparc64/kernel/us3_cpufreq.c-		cpufreq_us3_driver = NULL;
--
arch/sparc64/kernel/us3_cpufreq.c:		kfree(us3_freq_table);
arch/sparc64/kernel/us3_cpufreq.c-		us3_freq_table = NULL;
--
arch/x86_64/kernel/mce_amd.c:	kfree(per_cpu(threshold_banks, cpu)[bank]->blocks);
arch/x86_64/kernel/mce_amd.c-	per_cpu(threshold_banks, cpu)[bank]->blocks = NULL;
--
arch/x86_64/kernel/process.c:		kfree(t->io_bitmap_ptr);
arch/x86_64/kernel/process.c-		t->io_bitmap_ptr = NULL;
--
arch/x86_64/kernel/io_apic.c:			kfree(mp_ioapic_data[i]);
arch/x86_64/kernel/io_apic.c-			mp_ioapic_data[i] = NULL;
--
arch/arm/mach-s3c2410/pm.c:		kfree(crcs);
arch/arm/mach-s3c2410/pm.c-		crcs = NULL;
--
arch/arm/mach-sa1100/neponset.c:		kfree(dev->dev.power.saved_state);
arch/arm/mach-sa1100/neponset.c-		dev->dev.power.saved_state = NULL;
--
arch/arm/common/sa1111.c:		kfree(pdev->dev.power.saved_state);
arch/arm/common/sa1111.c-		pdev->dev.power.saved_state = NULL;
--
arch/arm/mach-netx/xc.c:	kfree(x);
arch/arm/mach-netx/xc.c-	x = NULL;
--
arch/sparc/kernel/of_device.c:		kfree(op);
arch/sparc/kernel/of_device.c-		op = NULL;
--
arch/i386/kernel/process.c:		kfree(t->io_bitmap_ptr);
arch/i386/kernel/process.c-		t->io_bitmap_ptr = NULL;
--
arch/i386/kernel/io_apic.c:		kfree(irq_cpu_data[i].irq_delta);
arch/i386/kernel/io_apic.c-		irq_cpu_data[i].irq_delta = NULL;
--
arch/i386/kernel/io_apic.c:		kfree(irq_cpu_data[i].last_irq);
arch/i386/kernel/io_apic.c-		irq_cpu_data[i].last_irq = NULL;
--
arch/i386/kernel/io_apic.c:			kfree(mp_ioapic_data[i]);
arch/i386/kernel/io_apic.c-			mp_ioapic_data[i] = NULL;
--
arch/i386/kernel/cpu/intel_cacheinfo.c:	kfree(cpuid4_info[cpu]);
arch/i386/kernel/cpu/intel_cacheinfo.c-	cpuid4_info[cpu] = NULL;
--
arch/i386/kernel/cpu/intel_cacheinfo.c:	kfree(index_kobject[cpu]);
arch/i386/kernel/cpu/intel_cacheinfo.c-	index_kobject[cpu] = NULL;
--
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c:				kfree(acpi_perf_data[j]);
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c-				acpi_perf_data[j] = NULL;
--
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c:		kfree(acpi_perf_data[j]);
arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c-		acpi_perf_data[j] = NULL;
--
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c:				kfree(acpi_perf_data[j]);
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c-				acpi_perf_data[j] = NULL;
--
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c:		kfree(acpi_perf_data[i]);
arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c-		acpi_perf_data[i] = NULL;
--
arch/i386/oprofile/nmi_int.c:		kfree(cpu_msrs[i].counters);
arch/i386/oprofile/nmi_int.c-		cpu_msrs[i].counters = NULL;
--
arch/i386/oprofile/nmi_int.c:		kfree(cpu_msrs[i].controls);
arch/i386/oprofile/nmi_int.c-		cpu_msrs[i].controls = NULL;
--
arch/h8300/platform/h8s/ints.c:		kfree(irq_list[irq]);
arch/h8300/platform/h8s/ints.c-		irq_list[irq] = NULL;
--
arch/h8300/kernel/ints.c:		kfree(irq_list[irq]);
arch/h8300/kernel/ints.c-		irq_list[irq] = NULL;
--
arch/um/kernel/irq.c:		kfree(tmp_pfd);
arch/um/kernel/irq.c-		tmp_pfd = NULL;
--
arch/um/drivers/daemon_user.c:		kfree(pri->local_addr);
arch/um/drivers/daemon_user.c-		pri->local_addr = NULL;
--
arch/um/drivers/daemon_user.c:	kfree(pri->data_addr);
arch/um/drivers/daemon_user.c-	pri->data_addr = NULL;
--
arch/um/drivers/daemon_user.c:	kfree(pri->ctl_addr);
arch/um/drivers/daemon_user.c-	pri->ctl_addr = NULL;
--
arch/um/drivers/daemon_user.c:	kfree(pri->local_addr);
arch/um/drivers/daemon_user.c-	pri->local_addr = NULL;
--
arch/um/drivers/net_kern.c:		kfree(*init_out);
arch/um/drivers/net_kern.c-		*init_out = NULL;
--
block/ll_rw_blk.c:		kfree(bqt->tag_index);
block/ll_rw_blk.c-		bqt->tag_index = NULL;
--
block/ll_rw_blk.c:		kfree(bqt->tag_map);
block/ll_rw_blk.c-		bqt->tag_map = NULL;
--
drivers/media/dvb/frontends/lnbp21.c:	kfree(fe->sec_priv);
drivers/media/dvb/frontends/lnbp21.c-	fe->sec_priv = NULL;
--
drivers/media/dvb/frontends/tua6100.c:	kfree(fe->tuner_priv);
drivers/media/dvb/frontends/tua6100.c-	fe->tuner_priv = NULL;
--
drivers/media/dvb/frontends/mt2060.c:	kfree(fe->tuner_priv);
drivers/media/dvb/frontends/mt2060.c-	fe->tuner_priv = NULL;
--
drivers/media/dvb/frontends/lgh06xf.c:	kfree(fe->tuner_priv);
drivers/media/dvb/frontends/lgh06xf.c-	fe->tuner_priv = NULL;
--
drivers/media/dvb/frontends/isl6421.c:	kfree(fe->sec_priv);
drivers/media/dvb/frontends/isl6421.c-	fe->sec_priv = NULL;
--
drivers/media/dvb/frontends/dvb-pll.c:	kfree(fe->tuner_priv);
drivers/media/dvb/frontends/dvb-pll.c-	fe->tuner_priv = NULL;
--
drivers/media/dvb/frontends/tda826x.c:	kfree(fe->tuner_priv);
drivers/media/dvb/frontends/tda826x.c-	fe->tuner_priv = NULL;
--
drivers/media/video/saa7134/saa7134-input.c:	kfree(dev->remote);
drivers/media/video/saa7134/saa7134-input.c-	dev->remote = NULL;
--
drivers/media/video/video-buf.c:			kfree(dma->sglist);
drivers/media/video/video-buf.c-			dma->sglist = NULL;
--
drivers/media/video/video-buf.c:	kfree(dma->sglist);
drivers/media/video/video-buf.c-	dma->sglist = NULL;
--
drivers/media/video/video-buf.c:		kfree(dma->pages);
drivers/media/video/video-buf.c-		dma->pages = NULL;
--
drivers/media/video/video-buf.c:	kfree(q->read_buf);
drivers/media/video/video-buf.c-	q->read_buf = NULL;
--
drivers/media/video/video-buf.c:			kfree (q->read_buf);
drivers/media/video/video-buf.c-			q->read_buf = NULL;
--
drivers/media/video/video-buf.c:		kfree(q->read_buf);
drivers/media/video/video-buf.c-		q->read_buf = NULL;
--
drivers/media/video/video-buf.c:		kfree(q->read_buf);
drivers/media/video/video-buf.c-		q->read_buf = NULL;
--
drivers/media/video/video-buf.c:		kfree(q->bufs[i]);
drivers/media/video/video-buf.c-		q->bufs[i] = NULL;
--
drivers/media/video/video-buf.c:		kfree(q->bufs[i]);
drivers/media/video/video-buf.c-		q->bufs[i] = NULL;
--
drivers/media/video/usbvideo/usbvideo.c:				kfree(uvd->sbuf[i].data);
drivers/media/video/usbvideo/usbvideo.c-				uvd->sbuf[i].data = NULL;
--
drivers/media/video/usbvideo/usbvideo.c:		kfree(uvd->sbuf[i].data);
drivers/media/video/usbvideo/usbvideo.c-		uvd->sbuf[i].data = NULL;
--
drivers/media/video/w9968cf.c:		kfree(cam->transfer_buffer[i]);
drivers/media/video/w9968cf.c-		cam->transfer_buffer[i] = NULL;
--
drivers/media/video/cafe_ccic.c:	kfree(cam->sb_bufs);
drivers/media/video/cafe_ccic.c-	cam->sb_bufs = NULL;
--
drivers/media/video/ov511.c:		kfree(ov->sbuf[i].data);
drivers/media/video/ov511.c-		ov->sbuf[i].data = NULL;
--
drivers/media/video/ov511.c:		kfree(ov->cbuf);
drivers/media/video/ov511.c-		ov->cbuf = NULL;
--
drivers/media/video/ov511.c:		kfree(ov);
drivers/media/video/ov511.c-		ov = NULL;
--
drivers/media/video/ov511.c:		kfree(ov->cbuf);
drivers/media/video/ov511.c-		ov->cbuf = NULL;
--
drivers/media/video/ov511.c:	kfree(ov);
drivers/media/video/ov511.c-	ov = NULL;
--
drivers/media/video/ov511.c:		kfree(ov->cbuf);
drivers/media/video/ov511.c-		ov->cbuf = NULL;
--
drivers/media/video/ov511.c:		kfree(ov);
drivers/media/video/ov511.c-		ov = NULL;
--
drivers/media/video/se401.c:		kfree(se401->scratch[i].data);
drivers/media/video/se401.c-		se401->scratch[i].data=NULL;
--
drivers/media/video/stv680.c:		kfree(stv680->scratch[i].data);
drivers/media/video/stv680.c-		stv680->scratch[i].data = NULL;
--
drivers/media/video/stv680.c:		kfree(stv680->sbuf[i].data);
drivers/media/video/stv680.c-		stv680->sbuf[i].data = NULL;
--
drivers/media/video/stv680.c:		kfree(stv680->scratch[i].data);
drivers/media/video/stv680.c-		stv680->scratch[i].data = NULL;
--
drivers/media/video/stv680.c:		kfree(stv680);
drivers/media/video/stv680.c-		stv680 = NULL;
--
drivers/media/video/stradis.c:	kfree(saa->dmavid2);
drivers/media/video/stradis.c-	saa->dmavid2 = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-ioread.c:		kfree(cp->sync_key_ptr);
drivers/media/video/pvrusb2/pvrusb2-ioread.c-		cp->sync_key_ptr = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-ioread.c:			kfree(cp->sync_key_ptr);
drivers/media/video/pvrusb2/pvrusb2-ioread.c-			cp->sync_key_ptr = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:		kfree(hdw->ctl_read_buffer);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-		hdw->ctl_read_buffer = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:		kfree(hdw->ctl_write_buffer);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-		hdw->ctl_write_buffer = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:		kfree(hdw->fw_buffer);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-		hdw->fw_buffer = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:		kfree(hdw->std_defs);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-		hdw->std_defs = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:		kfree(hdw->std_enum_names);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-		hdw->std_enum_names = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-hdw.c:			kfree(hdw->fw_buffer);
drivers/media/video/pvrusb2/pvrusb2-hdw.c-			hdw->fw_buffer = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-sysfs.c:	kfree(sfp->debugifc);
drivers/media/video/pvrusb2/pvrusb2-sysfs.c-	sfp->debugifc = NULL;
--
drivers/media/video/pvrusb2/pvrusb2-sysfs.c:		kfree(clp);
drivers/media/video/pvrusb2/pvrusb2-sysfs.c-		clp = NULL;
--
drivers/media/video/vivi.c:	kfree(buf->to_addr);
drivers/media/video/vivi.c-	buf->to_addr=NULL;
--
drivers/media/video/cx88/cx88-vp3054-i2c.c:		kfree(dev->card_priv);
drivers/media/video/cx88/cx88-vp3054-i2c.c-		dev->card_priv = NULL;
--
drivers/media/video/cpia_usb.c:	kfree (ucpia->sbuf[1].data);
drivers/media/video/cpia_usb.c-	ucpia->sbuf[1].data = NULL;
--
drivers/media/video/cpia_usb.c:	kfree (ucpia->sbuf[0].data);
drivers/media/video/cpia_usb.c-	ucpia->sbuf[0].data = NULL;
--
drivers/media/video/cpia_usb.c:	kfree(ucpia->sbuf[1].data);
drivers/media/video/cpia_usb.c-	ucpia->sbuf[1].data = NULL;
--
drivers/media/video/cpia_usb.c:	kfree(ucpia->sbuf[0].data);
drivers/media/video/cpia_usb.c-	ucpia->sbuf[0].data = NULL;
--
drivers/media/video/cpia2/cpia2_core.c:			kfree(cam->buffers);
drivers/media/video/cpia2/cpia2_core.c-			cam->buffers = NULL;
--
drivers/media/video/cpia2/cpia2_core.c:		kfree(cam->buffers);
drivers/media/video/cpia2/cpia2_core.c-		cam->buffers = NULL;
--
drivers/media/video/cpia2/cpia2_usb.c:			kfree(cam->sbuf[i].data);
drivers/media/video/cpia2/cpia2_usb.c-			cam->sbuf[i].data = NULL;
--
drivers/media/video/cpia2/cpia2_usb.c:				kfree(cam->sbuf[i].data);
drivers/media/video/cpia2/cpia2_usb.c-				cam->sbuf[i].data = NULL;
--
drivers/media/video/pwc/pwc-if.c:			kfree(pdev->sbuf[i].data);
drivers/media/video/pwc/pwc-if.c-			pdev->sbuf[i].data = NULL;
--
drivers/media/video/pwc/pwc-if.c:		kfree(pdev->fbuf);
drivers/media/video/pwc/pwc-if.c-		pdev->fbuf = NULL;
--
drivers/media/video/pwc/pwc-if.c:		kfree(pdev->decompress_data);
drivers/media/video/pwc/pwc-if.c-		pdev->decompress_data = NULL;
--
drivers/media/video/bt8xx/bttv-driver.c:				kfree(fh->ov.clips);
drivers/media/video/bt8xx/bttv-driver.c-			fh->ov.clips = NULL;
--
drivers/media/video/bt8xx/bttv-driver.c:				kfree (fh->cap.read_buf);
drivers/media/video/bt8xx/bttv-driver.c-				fh->cap.read_buf = NULL;
--
drivers/media/video/bt8xx/bttv-input.c:	kfree(btv->remote);
drivers/media/video/bt8xx/bttv-input.c-	btv->remote = NULL;
--
drivers/media/common/saa7146_core.c:		kfree(pt->slist);
drivers/media/common/saa7146_core.c-		pt->slist = NULL;
--
drivers/media/common/saa7146_core.c:	kfree(pt->slist);
drivers/media/common/saa7146_core.c-	pt->slist = NULL;
--
drivers/telephony/ixj.c:	kfree(j->fskdata);
drivers/telephony/ixj.c-	j->fskdata = NULL;
--
drivers/telephony/ixj.c:		kfree(j->read_buffer);
drivers/telephony/ixj.c-		j->read_buffer = NULL;
--
drivers/telephony/ixj.c:		kfree(j->write_buffer);
drivers/telephony/ixj.c-		j->write_buffer = NULL;
--
drivers/telephony/ixj.c:	kfree(j->read_buffer);
drivers/telephony/ixj.c-	j->read_buffer = NULL;
--
drivers/telephony/ixj.c:	kfree(j->write_buffer);
drivers/telephony/ixj.c-	j->write_buffer = NULL;
--
drivers/telephony/ixj.c:			kfree(j->cadence_t);
drivers/telephony/ixj.c-			j->cadence_t = NULL;
--
drivers/isdn/hisax/config.c:	kfree(cs->dlog);
drivers/isdn/hisax/config.c-	cs->dlog = NULL;
--
drivers/isdn/hisax/config.c:	kfree(csta->rcvbuf);
drivers/isdn/hisax/config.c-	csta->rcvbuf = NULL;
--
drivers/isdn/hisax/config.c:	kfree(cs);
drivers/isdn/hisax/config.c-	card->cs = NULL;
--
drivers/isdn/hisax/config.c:		kfree((void *) cards[cardnr].cs);
drivers/isdn/hisax/config.c-		cards[cardnr].cs = NULL;
--
drivers/isdn/hisax/hfc_2bs0.c:	kfree(cs->bcs[0].hw.hfc.send);
drivers/isdn/hisax/hfc_2bs0.c-	cs->bcs[0].hw.hfc.send = NULL;
--
drivers/isdn/hisax/hfc_2bs0.c:	kfree(cs->bcs[1].hw.hfc.send);
drivers/isdn/hisax/hfc_2bs0.c-	cs->bcs[1].hw.hfc.send = NULL;
--
drivers/isdn/hisax/netjet.c:				dev_kfree_skb_any(bcs->tx_skb);
drivers/isdn/hisax/netjet.c-				bcs->tx_skb = NULL;
--
drivers/isdn/hisax/netjet.c:		kfree(bcs->hw.tiger.rcvbuf);
drivers/isdn/hisax/netjet.c-		bcs->hw.tiger.rcvbuf = NULL;
--
drivers/isdn/hisax/netjet.c:		kfree(bcs->hw.tiger.sendbuf);
drivers/isdn/hisax/netjet.c-		bcs->hw.tiger.sendbuf = NULL;
--
drivers/isdn/hisax/netjet.c:			dev_kfree_skb_any(bcs->tx_skb);
drivers/isdn/hisax/netjet.c-			bcs->tx_skb = NULL;
--
drivers/isdn/hisax/netjet.c:	kfree(cs->bcs[0].hw.tiger.send);
drivers/isdn/hisax/netjet.c-	cs->bcs[0].hw.tiger.send = NULL;
--
drivers/isdn/hisax/netjet.c:	kfree(cs->bcs[0].hw.tiger.rec);
drivers/isdn/hisax/netjet.c-	cs->bcs[0].hw.tiger.rec = NULL;
--
drivers/isdn/hisax/isac.c:	kfree(cs->dc.isac.mon_rx);
drivers/isdn/hisax/isac.c-	cs->dc.isac.mon_rx = NULL;
--
drivers/isdn/hisax/isac.c:	kfree(cs->dc.isac.mon_tx);
drivers/isdn/hisax/isac.c-	cs->dc.isac.mon_tx = NULL;
--
drivers/isdn/hisax/hfc_2bds0.c:	kfree(cs->bcs[0].hw.hfc.send);
drivers/isdn/hisax/hfc_2bds0.c-	cs->bcs[0].hw.hfc.send = NULL;
--
drivers/isdn/hisax/hfc_2bds0.c:	kfree(cs->hw.hfcD.send);
drivers/isdn/hisax/hfc_2bds0.c-	cs->hw.hfcD.send = NULL;
--
drivers/isdn/hisax/avm_pci.c:		kfree(bcs->hw.hdlc.rcvbuf);
drivers/isdn/hisax/avm_pci.c-		bcs->hw.hdlc.rcvbuf = NULL;
--
drivers/isdn/hisax/avm_pci.c:		kfree(bcs->blog);
drivers/isdn/hisax/avm_pci.c-		bcs->blog = NULL;
--
drivers/isdn/hisax/avm_pci.c:			kfree(bcs->hw.hdlc.rcvbuf);
drivers/isdn/hisax/avm_pci.c-			bcs->hw.hdlc.rcvbuf = NULL;
--
drivers/isdn/hisax/hfc_pci.c:	kfree(cs->hw.hfcpci.share_start);
drivers/isdn/hisax/hfc_pci.c-	cs->hw.hfcpci.share_start = NULL;
--
drivers/isdn/hisax/icc.c:	kfree(cs->dc.icc.mon_rx);
drivers/isdn/hisax/icc.c-	cs->dc.icc.mon_rx = NULL;
--
drivers/isdn/hisax/icc.c:	kfree(cs->dc.icc.mon_tx);
drivers/isdn/hisax/icc.c-	cs->dc.icc.mon_tx = NULL;
--
drivers/isdn/hisax/w6692.c:		kfree(bcs->hw.w6692.rcvbuf);
drivers/isdn/hisax/w6692.c-		bcs->hw.w6692.rcvbuf = NULL;
--
drivers/isdn/hisax/w6692.c:		kfree(bcs->blog);
drivers/isdn/hisax/w6692.c-		bcs->blog = NULL;
--
drivers/isdn/hisax/w6692.c:			kfree(bcs->hw.w6692.rcvbuf);
drivers/isdn/hisax/w6692.c-			bcs->hw.w6692.rcvbuf = NULL;
--
drivers/isdn/hisax/ipacx.c:		kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/ipacx.c-		bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/ipacx.c:		kfree(bcs->blog);
drivers/isdn/hisax/ipacx.c-		bcs->blog = NULL;
--
drivers/isdn/hisax/ipacx.c:			kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/ipacx.c-			bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/elsa_ser.c:				kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/elsa_ser.c-			bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/elsa_ser.c:			dev_kfree_skb_any(bcs->tx_skb);
drivers/isdn/hisax/elsa_ser.c-			bcs->tx_skb = NULL;
--
drivers/isdn/hisax/elsa_ser.c:		kfree(cs->hw.elsa.rcvbuf);
drivers/isdn/hisax/elsa_ser.c-		cs->hw.elsa.rcvbuf = NULL;
--
drivers/isdn/hisax/elsa_ser.c:			kfree(cs->hw.elsa.rcvbuf);
drivers/isdn/hisax/elsa_ser.c-			cs->hw.elsa.rcvbuf = NULL;
--
drivers/isdn/hisax/elsa_ser.c:		kfree(cs->hw.elsa.transbuf);
drivers/isdn/hisax/elsa_ser.c-		cs->hw.elsa.transbuf = NULL;
--
drivers/isdn/hisax/callc.c:			kfree(csta->channel[i].b_st);
drivers/isdn/hisax/callc.c-			csta->channel[i].b_st = NULL;
--
drivers/isdn/hisax/isdnl3.c:		kfree(st->l3.global);
drivers/isdn/hisax/isdnl3.c-		st->l3.global = NULL;
--
drivers/isdn/hisax/st5481_usb.c:			kfree(urb[j]->transfer_buffer);
drivers/isdn/hisax/st5481_usb.c-			urb[j]->transfer_buffer = NULL;
--
drivers/isdn/hisax/hscx.c:		kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/hscx.c-		bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/hscx.c:		kfree(bcs->blog);
drivers/isdn/hisax/hscx.c-		bcs->blog = NULL;
--
drivers/isdn/hisax/hscx.c:			kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/hscx.c-			bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/jade.c:	kfree(bcs->hw.hscx.rcvbuf);
drivers/isdn/hisax/jade.c-	bcs->hw.hscx.rcvbuf = NULL;
--
drivers/isdn/hisax/jade.c:	kfree(bcs->blog);
drivers/isdn/hisax/jade.c-	bcs->blog = NULL;
--

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Al Viro]

On Mon, Jan 08, 2007 at 12:05:59AM -0800, Amit Choudhary wrote:
 
> Attached is some code from the kernel. Expanded KFREE() has been used atleast 1000 times in the
> kernel. By your logic, everyone is stupid in doing so. Something has been done atleast 1000 times
> in the kernel, that looks okay. But consolidating it at one place does not look okay. I am listing
> some of the 1000 places where KFREE() has been used. All this code have been written by atleast 50
> different people. From your logic they were doing "silly" things.

Very likely.  Some of that is a cargo-cult programming, some is explicit
logics controlling cleanup later on, some is outright racy (== everything
that leaves kfree()'d pointer in shared data structure for a while).

RE: [PATCH] include/linux/slab.h: new KFREE() macro.

[Hua Zhong]

> --- Hua Zhong <hzhong@gmail.com> wrote:
> 
> > > Any strong reason why not? x has some value that does not 
> > > make sense and can create only problems.
> > 
> > By the same logic, you should memset the buffer to zero 
> before freeing it too.
> > 
> 
> How does this help?

It doesn't. I thought that was my point?
 
> > > And as I explained, it can result in longer code too. So, why 
> > > keep this value around. Why not re-initialize it to NULL.
> > 
> > Because initialization increases code size.
> 
> Then why use kzalloc()? Let's remove _ALL_ the initialization 
> code from the kernel.

You initialize before use, not after.

> Attached is some code from the kernel. Expanded KFREE() has 
> been used atleast 1000 times in the
> kernel. By your logic, everyone is stupid in doing so. 
> Something has been done atleast 1000 times
> in the kernel, that looks okay. But consolidating it at one 
> place does not look okay. I am listing
> some of the 1000 places where KFREE() has been used. All this 
> code have been written by atleast 50
> different people. From your logic they were doing "silly" things.

Maybe you should first each one of them and see why they do it. 
And if there is no purpose than "better be safe than sorry", 
maybe you can then submit a patch to fix it.

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Bodo Eggert]

Amit Choudhary <amit2030@yahoo.com> wrote:
> --- Christoph Hellwig <hch@infradead.org> wrote:
>> On Sun, Jan 07, 2007 at 12:46:50AM -0800, Amit Choudhary wrote:

>> > Well, I am not proposing this as a debugging aid. The idea is about correct
>> > programming,
>> atleast
>> > from my view. Ideally, if you kfree(x), then you should set x to NULL. So,
>> > either programmers
>> do
>> > it themselves or a ready made macro do it for them.
>> 
>> No, you should not.  I suspect that's the basic point you're missing.

> Any strong reason why not? x has some value that does not make sense and can
> create only problems. And as I explained, it can result in longer code too.
> So, why keep this value around. Why not re-initialize it to NULL.

1) Because some magic value like 0x23 would be better.
2) Because it hides bugs like double frees or dangeling references while
   creating a race condition. In the end, you'll get more hard-to-find bugs
   in exchange for papering over some easy-to-spot bugs.

> If x should not be re-initialized to NULL, then by the same logic, we should
> not even initialize local variables. And all of us know that local variables
> should be initialized.

That may hide bugs, too. Therefore this isn't done in the kernel unless you
intend to depend on an initial value.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html

Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Amit Choudhary]

>Amit Choudhary <amit2030@xxxxxxxxx> wrote:
>> --- Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>>> On Sun, Jan 07, 2007 at 12:46:50AM -0800, Amit Choudhary wrote:

>> Any strong reason why not? x has some value that does not make sense and can
>> create only problems. And as I explained, it can result in longer code too.
>> So, why keep this value around. Why not re-initialize it to NULL.

>1) Because some magic value like 0x23 would be better.
>2) Because it hides bugs like double frees or dangeling references while
>creating a race condition. In the end, you'll get more hard-to-find bugs
>in exchange for papering over some easy-to-spot bugs.

Sorry for spamming the list again but some people may find this useful:

[Dangling pointers are security vulnerability]

http://it.slashdot.org/it/07/07/23/1624203.shtml

Excerpt:
***
Dangling pointers are quite common, but security experts and developers have said for years that
there is no practical way to exploit them, so they've been considered quality-assurance problems
and not security flaws.

But now that has changed.

"The problem before was, you had to override the exact location that the pointer was pointing to.
It was considered impossible. But we discovered a way to do this with generic dangling pointers
and run our own shell code."
***

-Amit



       
____________________________________________________________________________________
Take the Internet to Go: Yahoo!Go puts the Internet in your pocket: mail, news, photos & more. 
http://mobile.yahoo.com/go?refer=1GNXIC