Re: 2.6.17-rc5-mm1

[Hugh Dickins <hugh@veritas.com>]

On Mon, 5 Jun 2006, Andrew Morton wrote:
> On Mon, 5 Jun 2006 13:43:47 -0700 (PDT)
> Christoph Lameter <clameter@sgi.com> wrote:
> 
> > Fix this by limiting the number of swap devices in swapon to 
> > MAX_SWAPFILES
> > 
> > Signed-off-by: Christoph Lameter <clameter@sgi.com>
> > 
> > Index: linux-2.6.17-rc5-mm2/mm/swapfile.c
> > ===================================================================
> > --- linux-2.6.17-rc5-mm2.orig/mm/swapfile.c	2006-06-01 10:03:07.127259731 -0700
> > +++ linux-2.6.17-rc5-mm2/mm/swapfile.c	2006-06-05 13:40:45.887291175 -0700
> > @@ -1408,8 +1408,13 @@ asmlinkage long sys_swapon(const char __
> >  		spin_unlock(&swap_lock);
> >  		goto out;
> >  	}
> > -	if (type >= nr_swapfiles)
> > +	if (type >= nr_swapfiles) {
> > +		if (nr_swapfiles >= MAX_SWAPFILES) {
> > +			spin_unlock(&swap_lock);
> > +			goto out;
> > +		}
> >  		nr_swapfiles = type+1;
> > +	}
> >  	INIT_LIST_HEAD(&p->extent_list);
> >  	p->flags = SWP_USED;
> >  	p->swap_file = NULL;
> 
> Thanks, Christoph.  So we get to keep the crappy test?

Christoph's patch looks like it will fix the corruption to me (though
I'd have thought the alternative below a little cleaner, keeping the
associated tests together and avoiding a second unlock: whatever).

Whether it's correct depends on what Martin was trying to achieve
with his test.  I'm surprised to find a very ordinary
#define __swp_type(entry)	(((entry).val >> 2) & 0x1f)
in include/asm-s390/pgtable.h, and no architecture with a more
limiting mask.

I had expected that s390 (or some other) was short of bits and was
having to use a more limiting mask: in which case neither Christoph's
patch nor the one below would be appropriate, since the migration
types 30 and 31 would then fall outside the mask (though it'd only
be a problem if page migration were required on such an architecture).

So, the patch should do for now, but it'd be helpful if Martin can
explain why he needed to use that test in 2.6.5.  (Perhaps he went
through a phase of reducing the swap type and extending the swap
offset, and this is a remnant of that phase?)

> I wonder what LTP was corrupting before it started to corrupt page
> migration data?
> 
> The above looks like a 2.6.17 thing to me.

Not really (though the clarity and reassurance of the additional
MAX_SWAPFILES test is good).  We went over it a year or two back,
and the macro contortions do involve MAX_SWAPFILES_SHIFT: which
up to and including 2.6.17 has enforced the MAX_SWAPFILES limit.

But swapless migration has changed the relationship between
MAX_SWAPFILES_SHIFT and MAX_SWAPFILES (-2 when CONFIG_MIGRATION),
hence this corruption now surfacing in -mm.

Hugh

--- 2.6.17-rc5-mm3/mm/swapfile.c	2006-06-04 11:52:47.000000000 +0100
+++ linux/mm/swapfile.c	2006-06-06 05:23:05.000000000 +0100
@@ -1404,7 +1404,8 @@ asmlinkage long sys_swapon(const char __
 	 * from the initial ~0UL that can't be encoded in either the
 	 * swp_entry_t or the architecture definition of a swap pte.
 	 */
-	if (type > swp_type(pte_to_swp_entry(swp_entry_to_pte(swp_entry(~0UL,0))))) {
+	if (type >= MAX_SWAPFILES ||
+	    type > swp_type(pte_to_swp_entry(swp_entry_to_pte(swp_entry(~0UL,0))))) {
 		spin_unlock(&swap_lock);
 		goto out;
 	}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/