Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

["Rafael J. Wysocki" <rjw@sisk.pl>]

Hi,

On Monday 27 February 2006 19:53, Jesper Juhl wrote:
> On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Make shrink_all_memory() overflow-resistant.
> >
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  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