Re: [PATCH] include/linux/slab.h: new KFREE() macro.
>Amit Choudhary <[EMAIL PROTECTED]> wrote: >> --- Christoph Hellwig <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
Amit Choudhary [EMAIL PROTECTED] wrote: --- Christoph Hellwig [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Randy Dunlap <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. pgpWOxg5tzf1e.pgp Description: PGP signature
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- [EMAIL PROTECTED] 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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.
pgphGdH90pjgg.pgp
Description: PGP signature
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On Tue, 9 Jan 2007 11:02:35 -0800 (PST) Amit Choudhary wrote: > > --- [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On Tue, 9 Jan 2007 11:02:35 -0800 (PST) Amit Choudhary wrote: --- [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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.
pgphGdH90pjgg.pgp
Description: PGP signature
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- [EMAIL PROTECTED] 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. pgpWOxg5tzf1e.pgp Description: PGP signature
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Randy Dunlap [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 pgpLgKhZ0oKlx.pgp Description: PGP signature
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
Amit Choudhary <[EMAIL PROTECTED]> wrote: > --- Christoph Hellwig <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 08/01/07, Amit Choudhary <[EMAIL PROTECTED]> wrote: --- Pekka Enberg <[EMAIL PROTECTED]> wrote: > On 1/8/07, Hua Zhong <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/8/07, Amit Choudhary <[EMAIL PROTECTED]> 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). - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Pekka Enberg <[EMAIL PROTECTED]> wrote: > Hi Amit, > > On 1/8/07, Amit Choudhary <[EMAIL PROTECTED]> 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 - 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/
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
> --- Hua Zhong <[EMAIL PROTECTED]> 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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Sumit Narayan <[EMAIL PROTECTED]> 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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)
oooh ... 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 ...
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
Hi Amit, On 1/8/07, Amit Choudhary <[EMAIL PROTECTED]> 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? - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 <[EMAIL PROTECTED]> wrote:
--- Pekka Enberg <[EMAIL PROTECTED]> wrote:
> On 1/8/07, Hua Zhong <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On Mon, Jan 08, 2007 at 12:31:44AM -0800, Amit Choudhary wrote: > > --- Pekka Enberg <[EMAIL PROTECTED]> wrote: > > > On 1/8/07, Hua Zhong <[EMAIL PROTECTED]> 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... - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Pekka Enberg <[EMAIL PROTECTED]> wrote: > On 1/8/07, Hua Zhong <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] 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.
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). - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 - 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/
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Hua Zhong <[EMAIL PROTECTED]> 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]);
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/8/07, Hua Zhong <[EMAIL PROTECTED]> 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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/8/07, Hua Zhong [EMAIL PROTECTED] 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. - 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/
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Hua Zhong [EMAIL PROTECTED] 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; --
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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). - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Pekka Enberg [EMAIL PROTECTED] wrote: On 1/8/07, Hua Zhong [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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.
On Mon, Jan 08, 2007 at 12:31:44AM -0800, Amit Choudhary wrote: --- Pekka Enberg [EMAIL PROTECTED] wrote: On 1/8/07, Hua Zhong [EMAIL PROTECTED] 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... - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 [EMAIL PROTECTED] wrote:
--- Pekka Enberg [EMAIL PROTECTED] wrote:
On 1/8/07, Hua Zhong [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
Hi Amit, On 1/8/07, Amit Choudhary [EMAIL PROTECTED] 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? - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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)
oooh ... 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 ...
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Sumit Narayan [EMAIL PROTECTED] 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
-
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/
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Hua Zhong [EMAIL PROTECTED] 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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Pekka Enberg [EMAIL PROTECTED] wrote: Hi Amit, On 1/8/07, Amit Choudhary [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/8/07, Amit Choudhary [EMAIL PROTECTED] 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). - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 08/01/07, Amit Choudhary [EMAIL PROTECTED] wrote: --- Pekka Enberg [EMAIL PROTECTED] wrote: On 1/8/07, Hua Zhong [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
Amit Choudhary [EMAIL PROTECTED] wrote: --- Christoph Hellwig [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 pgpLgKhZ0oKlx.pgp Description: PGP signature
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
> 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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov <[EMAIL PROTECTED]> 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
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov <[EMAIL PROTECTED]> 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Christoph Hellwig <[EMAIL PROTECTED]> 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
>>On 1/1/07, Amit Choudhary <[EMAIL PROTECTED]> 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/1/07, Amit Choudhary [EMAIL PROTECTED] 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Christoph Hellwig [EMAIL PROTECTED] 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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov [EMAIL PROTECTED] 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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov [EMAIL PROTECTED] 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;
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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 - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
--- Vadim Lobanov [EMAIL PROTECTED] 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 - 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/
RE: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
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. - 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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/1/07, Amit Choudhary <[EMAIL PROTECTED]> 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.
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
On 1/1/07, Amit Choudhary [EMAIL PROTECTED] 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.
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
+#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
-
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/
[PATCH] include/linux/slab.h: new KFREE() macro.
Description: new KFREE() macro to set the variable NULL after freeing it.
Signed-off-by: Amit Choudhary <[EMAIL PROTECTED]>
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.
-
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/
[PATCH] include/linux/slab.h: new KFREE() macro.
Description: new KFREE() macro to set the variable NULL after freeing it.
Signed-off-by: Amit Choudhary [EMAIL PROTECTED]
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.
-
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/
Re: [PATCH] include/linux/slab.h: new KFREE() macro.
+#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
-
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/
