Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 09, 2013 at 03:00:54PM -0800, Christopher Li wrote: > On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter > wrote: > > The problems is if we go over the 8k stack. So big arrays are bad. > > Also if the dynamically sized array is inside a loop then normally > > GCC frees it after each iteration, but on some arches it didn't free > > it until after the last iteration. > > So it seems that you agree those variable array usage should be > better change to use kmalloc or some thing. > > > Btw, I've Smatch has cross function analysis, and I'd like to use > > it here to figure out if the max size for dynamically sized arrays. > > I ran into a problem: > > > > The code looks like this: > > char buf[a]; > > The size expression should be an EXPR_SYMBOL, but smatch gets: > > char buf[*a]; > > Sparse currently does not deal with the dynamic array size right now. > It only want to get constant value from the array size. > > The part that evaluate the array size is actually correct. Remember > the EXPR_SYMBOL > actually contain the *address* of symbol "a". So the proper > sizeof(buf) is actually > the content of "*a". That part is fine. It's evaluating it correctly, but Smatch normally expects expressions which haven't been evaluated yet. I can probably hack my own Sparse tree for what I need. It's not a big deal. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 09, 2013 at 03:00:54PM -0800, Christopher Li wrote: On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter dan.carpen...@oracle.com wrote: The problems is if we go over the 8k stack. So big arrays are bad. Also if the dynamically sized array is inside a loop then normally GCC frees it after each iteration, but on some arches it didn't free it until after the last iteration. So it seems that you agree those variable array usage should be better change to use kmalloc or some thing. Btw, I've Smatch has cross function analysis, and I'd like to use it here to figure out if the max size for dynamically sized arrays. I ran into a problem: The code looks like this: char buf[a]; The size expression should be an EXPR_SYMBOL, but smatch gets: char buf[*a]; Sparse currently does not deal with the dynamic array size right now. It only want to get constant value from the array size. The part that evaluate the array size is actually correct. Remember the EXPR_SYMBOL actually contain the *address* of symbol a. So the proper sizeof(buf) is actually the content of *a. That part is fine. It's evaluating it correctly, but Smatch normally expects expressions which haven't been evaluated yet. I can probably hack my own Sparse tree for what I need. It's not a big deal. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter wrote:
> The problems is if we go over the 8k stack. So big arrays are bad.
> Also if the dynamically sized array is inside a loop then normally
> GCC frees it after each iteration, but on some arches it didn't free
> it until after the last iteration.
So it seems that you agree those variable array usage should be
better change to use kmalloc or some thing.
> Btw, I've Smatch has cross function analysis, and I'd like to use
> it here to figure out if the max size for dynamically sized arrays.
> I ran into a problem:
>
> The code looks like this:
> char buf[a];
> The size expression should be an EXPR_SYMBOL, but smatch gets:
> char buf[*a];
Sparse currently does not deal with the dynamic array size right now.
It only want to get constant value from the array size.
The part that evaluate the array size is actually correct. Remember
the EXPR_SYMBOL
actually contain the *address* of symbol "a". So the proper
sizeof(buf) is actually
the content of "*a". That part is fine.
The more complicated case of dynamic array size is using the dynamic array in
a struct:
struct {
char descriptor1[length+1];
char descriptor2[length+1];
} *d;
Then the sizeof(*d) need to be ((*length) + 1 + (*length) + 1), assume
"length" is a
symbol address. The sizeof (struct foo) can be pretty complicate expression.
Some USB code use this kind of the dynamic array. However, it does not allocate
the struct in the stack, the struct is allocated via kmalloc using pointer.
Sparse still complain the variable length array though.
Let me see if I can make the sparse handle dynamic array better.
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 09, 2013 at 10:10:08AM -0800, Christopher Li wrote: > On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter > wrote: > > On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: > >> Roughly how many instances of this are there kernel-wide? > >> > > > > Around 150 on x86 allmodconfig. They are pretty well audited. > > I saw 207 on x86-64 allmodconfig. See the list that I attached. > Ah. Sorry, I'm on my laptop and my sparse output was old. > Can you elaborate the well audited part? How it was audited? > The problems is if we go over the 8k stack. So big arrays are bad. Also if the dynamically sized array is inside a loop then normally GCC frees it after each iteration, but on some arches it didn't free it until after the last iteration. Btw, I've Smatch has cross function analysis, and I'd like to use it here to figure out if the max size for dynamically sized arrays. I ran into a problem: The code looks like this: char buf[a]; The size expression should be an EXPR_SYMBOL, but smatch gets: char buf[*a]; Where the size expression is an EXPR_PREOP. In smatch, how I use sparse is that I call sparse_keep_tokens() and then I parse the resulting symbol list myself. The problem is in examine_array_type() we call get_expression_value() which changes the symbols from normal symbols to dereferences. The call tree is: examine_array_type() -> get_expression_value() -> __get_expression_value() -> evaluate_expression() -> evaluate_symbol_expression() <- change happens here. I'm not sure what to do. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter wrote: > On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: >> Roughly how many instances of this are there kernel-wide? >> > > Around 150 on x86 allmodconfig. They are pretty well audited. I saw 207 on x86-64 allmodconfig. See the list that I attached. Can you elaborate the well audited part? How it was audited? I try to figure out if it is worth the trouble to fix it. Chris var-array-error Description: Binary data
Re: Suggestion for fixing the variable length array used in the kernel.
On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter dan.carpen...@oracle.com wrote: On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: Roughly how many instances of this are there kernel-wide? Around 150 on x86 allmodconfig. They are pretty well audited. I saw 207 on x86-64 allmodconfig. See the list that I attached. Can you elaborate the well audited part? How it was audited? I try to figure out if it is worth the trouble to fix it. Chris var-array-error Description: Binary data
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 09, 2013 at 10:10:08AM -0800, Christopher Li wrote: On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter dan.carpen...@oracle.com wrote: On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: Roughly how many instances of this are there kernel-wide? Around 150 on x86 allmodconfig. They are pretty well audited. I saw 207 on x86-64 allmodconfig. See the list that I attached. Ah. Sorry, I'm on my laptop and my sparse output was old. Can you elaborate the well audited part? How it was audited? The problems is if we go over the 8k stack. So big arrays are bad. Also if the dynamically sized array is inside a loop then normally GCC frees it after each iteration, but on some arches it didn't free it until after the last iteration. Btw, I've Smatch has cross function analysis, and I'd like to use it here to figure out if the max size for dynamically sized arrays. I ran into a problem: The code looks like this: char buf[a]; The size expression should be an EXPR_SYMBOL, but smatch gets: char buf[*a]; Where the size expression is an EXPR_PREOP. In smatch, how I use sparse is that I call sparse_keep_tokens() and then I parse the resulting symbol list myself. The problem is in examine_array_type() we call get_expression_value() which changes the symbols from normal symbols to dereferences. The call tree is: examine_array_type() - get_expression_value() - __get_expression_value() - evaluate_expression() - evaluate_symbol_expression() - change happens here. I'm not sure what to do. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter dan.carpen...@oracle.com wrote:
The problems is if we go over the 8k stack. So big arrays are bad.
Also if the dynamically sized array is inside a loop then normally
GCC frees it after each iteration, but on some arches it didn't free
it until after the last iteration.
So it seems that you agree those variable array usage should be
better change to use kmalloc or some thing.
Btw, I've Smatch has cross function analysis, and I'd like to use
it here to figure out if the max size for dynamically sized arrays.
I ran into a problem:
The code looks like this:
char buf[a];
The size expression should be an EXPR_SYMBOL, but smatch gets:
char buf[*a];
Sparse currently does not deal with the dynamic array size right now.
It only want to get constant value from the array size.
The part that evaluate the array size is actually correct. Remember
the EXPR_SYMBOL
actually contain the *address* of symbol a. So the proper
sizeof(buf) is actually
the content of *a. That part is fine.
The more complicated case of dynamic array size is using the dynamic array in
a struct:
struct {
char descriptor1[length+1];
char descriptor2[length+1];
} *d;
Then the sizeof(*d) need to be ((*length) + 1 + (*length) + 1), assume
length is a
symbol address. The sizeof (struct foo) can be pretty complicate expression.
Some USB code use this kind of the dynamic array. However, it does not allocate
the struct in the stack, the struct is allocated via kmalloc using pointer.
Sparse still complain the variable length array though.
Let me see if I can make the sparse handle dynamic array better.
Chris
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: > On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li wrote: > > > Hi, > > > > I am looking at the current sparse warning on the kernel source. > > One category of those warning are produce by the variable length array. > > We all know that the kernel stack has a limit so we don't want to allocate > > too much stack to the variable size array. > > > > Is there a recommended way to fix those warnings? Is it worth while to > > fix it at all? I am looking forward to some kind of guideline how to handle > > this. > > Roughly how many instances of this are there kernel-wide? > Around 150 on x86 allmodconfig. They are pretty well audited. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li wrote: > Hi, > > I am looking at the current sparse warning on the kernel source. > One category of those warning are produce by the variable length array. > We all know that the kernel stack has a limit so we don't want to allocate > too much stack to the variable size array. > > Is there a recommended way to fix those warnings? Is it worth while to > fix it at all? I am looking forward to some kind of guideline how to handle > this. Roughly how many instances of this are there kernel-wide? I don't think it's good practice in the kernel - it's somewhat dangerous and the effects of errors will be catastrophic. And as you've seen, those sites are difficult to review for safety. We could just outright ban the thing and convert those sites to kmalloc() or whatever. If people howl about the performance impact (unlikely) then perhaps we can put something together using __builtin_alloca() which includes runtime checking for "excessive" allocations. If an excessive allocation is detected we'd warn and return NULL. Anyway, yes, variable-length arrays are problematic so for now, let's leave the sparse warnings in place? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li spa...@chrisli.org wrote: Hi, I am looking at the current sparse warning on the kernel source. One category of those warning are produce by the variable length array. We all know that the kernel stack has a limit so we don't want to allocate too much stack to the variable size array. Is there a recommended way to fix those warnings? Is it worth while to fix it at all? I am looking forward to some kind of guideline how to handle this. Roughly how many instances of this are there kernel-wide? I don't think it's good practice in the kernel - it's somewhat dangerous and the effects of errors will be catastrophic. And as you've seen, those sites are difficult to review for safety. We could just outright ban the thing and convert those sites to kmalloc() or whatever. If people howl about the performance impact (unlikely) then perhaps we can put something together using __builtin_alloca() which includes runtime checking for excessive allocations. If an excessive allocation is detected we'd warn and return NULL. Anyway, yes, variable-length arrays are problematic so for now, let's leave the sparse warnings in place? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Suggestion for fixing the variable length array used in the kernel.
On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote: On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li spa...@chrisli.org wrote: Hi, I am looking at the current sparse warning on the kernel source. One category of those warning are produce by the variable length array. We all know that the kernel stack has a limit so we don't want to allocate too much stack to the variable size array. Is there a recommended way to fix those warnings? Is it worth while to fix it at all? I am looking forward to some kind of guideline how to handle this. Roughly how many instances of this are there kernel-wide? Around 150 on x86 allmodconfig. They are pretty well audited. regards, dan carpenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
