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.
Some of them has estimated size limited, like the one fournd in decode_rs.c
/* Err+Eras Locator poly and syndrome poly The maximum value
* of nroots is 8. So the necessary stack size will be about
* 220 bytes max.
*/
uint16_t lambda[nroots + 1], syn[nroots];
uint16_t b[nroots + 1], t[nroots + 1], omega[nroots + 1];
uint16_t root[nroots], reg[nroots + 1], loc[nroots];
Some of them did not said the size estimation but you kind of know
they are not likely to blow up the stack:
In xen_flush_tlb_others
struct {
struct mmuext_op op;
#ifdef CONFIG_SMP
DECLARE_BITMAP(mask, num_processors);
#else
DECLARE_BITMAP(mask, NR_CPUS);
#endif
} *args;
And also some of them are harder to tell from the context if
there will be a size limit:
int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
unsigned int k;
struct snd_pcm_hardware *hw;
struct snd_interval *i = NULL;
struct snd_mask *m = NULL;
struct snd_pcm_hw_constraints *constrs = &substream->runtime->hw_constraints;
unsigned int rstamps[constrs->rules_num]; <---------------------------------
Chris
On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li <[email protected]> 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?
On Fri, Mar 08, 2013 at 04:29:22PM -0800, Andrew Morton wrote:
> On Wed, 6 Mar 2013 20:46:35 -0800 Christopher Li <[email protected]> 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
On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter <[email protected]> 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
On Sat, Mar 09, 2013 at 10:10:08AM -0800, Christopher Li wrote:
> On Fri, Mar 8, 2013 at 9:39 PM, Dan Carpenter <[email protected]> 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
On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter <[email protected]> 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
On Sat, Mar 09, 2013 at 03:00:54PM -0800, Christopher Li wrote:
> On Sat, Mar 9, 2013 at 2:34 PM, Dan Carpenter <[email protected]> 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