Hi,
On Monday 27 February 2006 19:53, Jesper Juhl wrote:
> On 2/27/06, Rafael J. Wysocki <[email protected]> wrote:
> > Make shrink_all_memory() overflow-resistant.
> >
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > include/linux/swap.h | 2 +-
> > mm/vmscan.c | 9 +++++----
> > 2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> > +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> > @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
> > * Try to free `nr_pages' of memory, system-wide. Returns the number of freed
> > * pages.
> > */
> > -int shrink_all_memory(unsigned long nr_pages)
> > +unsigned long shrink_all_memory(unsigned int nr_pages)
>
> What about the callers of shrink_all_memory() who currently expect it
> to return an int, have you checked how they will react to you changing
> the return type (and signedness) ?
AFAICT, there are not any.
> > {
> > pg_data_t *pgdat;
> > - unsigned long nr_to_free = nr_pages;
> > - int ret = 0;
> > + long long nr_to_free = nr_pages;
>
> 'nr_pages' passed to the function is an unsigned int, why this change?
> also, nr_to_free is later passed to balance_pgdat() as the second
> argument and balance_pgdat() expects to be passed an int.
First, the balance_pgdat() in the current -mm expects to be passed
unsigned long here.
Second, say you pass 100 and balance_pgdat() returns 110. Then if
nr_to_free is unsigned, it will become a huge positive number instead of
a negative number, as it should be. Thus it has to be signed IMO.
It could be long, but on i386 long is the same as int.
> > + unsigned long ret = 0;
> > struct reclaim_state reclaim_state = {
> > .reclaimed_slab = 0,
> > };
> >
> > current->reclaim_state = &reclaim_state;
> > for_each_pgdat(pgdat) {
> > - int freed;
> > + unsigned long freed;
>
> balance_pgdat() returns a plain int, so what's the point of making
> 'freed' an unsigned long?
Well, at least the balance_pgdat() I'm looking at now returns
unsigned long.
> > +
> > freed = balance_pgdat(pgdat, nr_to_free, 0);
> > ret += freed;
> > nr_to_free -= freed;
> > Index: linux-2.6.16-rc4-mm2/include/linux/swap.h
> > ===================================================================
> > --- linux-2.6.16-rc4-mm2.orig/include/linux/swap.h
> > +++ linux-2.6.16-rc4-mm2/include/linux/swap.h
> > @@ -173,7 +173,7 @@ extern void swap_setup(void);
> >
> > /* linux/mm/vmscan.c */
> > extern unsigned long try_to_free_pages(struct zone **, gfp_t);
> > -extern int shrink_all_memory(unsigned long nr_pages);
> > +extern unsigned long shrink_all_memory(unsigned int nr_pages);
> > extern int vm_swappiness;
> >
> > #ifdef CONFIG_NUMA
> >
>
>
> This patch may be correct or it may be wrong, I've not spend enough
> time staring at it and follow the code it calls and gets called by to
> say which, but I for one would like an explanation of why these
> changes are made and why they are correct.
> There's a distinct lack of a changelog/explanation with this patch.
> Or maybe I'm just dense and can't see the obvious correctness, but if
> that's the case I'd still like to be enlightened :)
Perhaps I should have explained it in a more detailed way in the
changelog.
The problem is that, AFAICT, balance_pgdat(pgdat, nr_to_free, 0) may free
more than nr_to_free pages, in which case nr_to_free, being unsigned, will
overflow (and obviously it cannot be less than 0). Also if nr_pages is
too big, strange things may happen (without the patch, that is).
Greetings,
Rafael
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
