Re: [PATCH, take2] netfilter : struct xt_table_info diet

[Eric Dumazet <dada1@cosmosbay.com>]

On Thu, 15 Nov 2007 13:41:54 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Eric Dumazet wrote:
> > [PATCH] netfilter : struct xt_table_info diet
> > 
> > Instead of using a big array of NR_CPUS entries, we can compute the size
> > needed at runtime, using nr_cpu_ids
> > 
> > This should save some ram (especially on David's machines where
> > NR_CPUS=4096 :
> > 32 KB can be saved per table, and 64KB for dynamically allocated ones
> > (because
> > of slab/slub alignements) )
> > 
> > In particular, the 'bootstrap' tables are not any more static (in data
> > section) but on stack as their size is now very small.
> > 
> > This also should reduce the size used on stack in compat functions
> > (get_info() declares an automatic variable, that could be bigger than
> > kernel
> > stack size for big NR_CPUS)
> 
> 
> I fixed a compilation error with CONFIG_COMPAT and applied it, thanks
> Eric. One question though:
> 
> > +#define XT_TABLE_INFO_SZ (offsetof(struct xt_table_info, entries) \
> > +			  + nr_cpu_ids * sizeof(char *))
> 
> 
> >  	/* overflow check */
> > -	if (tmp.size >= (INT_MAX - sizeof(struct xt_table_info)) / NR_CPUS -
> > -			SMP_CACHE_BYTES)
> > +	if (tmp.size >= INT_MAX / num_possible_cpus())
> >  		return -ENOMEM;
> 
> We need to make sure offsetof(struct xt_table_info, entries) +
> nr_cpu_ids * sizeof(char *) doesn't overflow, so why doesn't it
> use nr_cpu_ids here as well?
> 

nr_cpu_ids is <= NR_CPUS, so XT_TABLE_INFO_SZ cannot overflow

The 'overflow check' we do here is in fact not very usefull now
that we dont need to multiply tmp.size by NR_CPUS and potentially
overflow the result.

We can delete the test, because kmalloc()/vmalloc() will probably
fail gracefully if we ask too much memory.

We could imagine a dual Opteron machine, with a total of 32GB of ram, and
it could be possible to load a 3GB iptable  (that would consume 2*3GB of ram), 
but the 'overflow check' test actually forbids such a scenario.