[RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

The ksize() function is meant for objects allocated from the slab caches, not
for arbitrary objects. Add a BUG_ON() to enforce that.

Cc: Christoph Lameter <clameter@sgi.com>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slab.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 06236e4..b98bb7c 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4472,10 +4472,14 @@ const struct seq_operations slabstats_op = {
  */
 size_t ksize(const void *objp)
 {
+	struct page *page;
+
 	BUG_ON(!objp);
 	if (unlikely(objp == ZERO_SIZE_PTR))
 		return 0;
 
-	return obj_size(virt_to_cache(objp));
+	page = virt_to_page(objp);
+	BUG_ON(!PageSlab(page));
+	return obj_size(page_get_cache(page));
 }
 EXPORT_SYMBOL(ksize);
-- 
1.5.2.5

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Paul Mundt]

On Wed, May 21, 2008 at 09:25:26PM +0300, Pekka J Enberg wrote:
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> The ksize() function is meant for objects allocated from the slab caches, not
> for arbitrary objects. Add a BUG_ON() to enforce that.
> 
> Cc: Christoph Lameter <clameter@sgi.com>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

This series seems to work ok with all of SLAB/SLUB/SLOB when also
applying the kobjsize() patch. Without the kobjsize() patch in place,
SLAB and SLUB both work, while SLOB still triggers the >= MAX_ORDER
BUG_ON() for page cache pages. Note that this is still an improvement
over SLUB blowing up, as it was before, even without the kobjsize()
change.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Pekka Enberg]

Paul Mundt wrote:
> This series seems to work ok with all of SLAB/SLUB/SLOB when also
> applying the kobjsize() patch. Without the kobjsize() patch in place,
> SLAB and SLUB both work, while SLOB still triggers the >= MAX_ORDER
> BUG_ON() for page cache pages. Note that this is still an improvement
> over SLUB blowing up, as it was before, even without the kobjsize()
> change.

That's probably due to the fact that we use PageSlab for all pages with 
SLUB and for bigblock pages in SLOB with this patch. Christoph, Matt, 
any suggestions how to fix the PageSlab check in nommu btw?

		Pekka

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Christoph Lameter]

On Thu, 22 May 2008, Pekka Enberg wrote:

> Paul Mundt wrote:
> > This series seems to work ok with all of SLAB/SLUB/SLOB when also
> > applying the kobjsize() patch. Without the kobjsize() patch in place,
> > SLAB and SLUB both work, while SLOB still triggers the >= MAX_ORDER
> > BUG_ON() for page cache pages. Note that this is still an improvement
> > over SLUB blowing up, as it was before, even without the kobjsize()
> > change.
> 
> That's probably due to the fact that we use PageSlab for all pages with SLUB
> and for bigblock pages in SLOB with this patch. Christoph, Matt, any
> suggestions how to fix the PageSlab check in nommu btw?

I already posted a patch and commented numerous times. What else needs 
to be said? SLUB only uses PageSlab for kmalloc <=4kb.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Pekka Enberg]

Christoph Lameter wrote:
>> That's probably due to the fact that we use PageSlab for all pages with SLUB
>> and for bigblock pages in SLOB with this patch. Christoph, Matt, any
>> suggestions how to fix the PageSlab check in nommu btw?
> 
> I already posted a patch and commented numerous times. What else needs 
> to be said? SLUB only uses PageSlab for kmalloc <=4kb.

(I missed your patch, btw.)

For SLOB, we will never use ksize() as it doesn't set PageSlab. Is that 
not a problem?

		Pekka

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Paul Mundt]

On Thu, May 22, 2008 at 07:21:48AM +0300, Pekka Enberg wrote:
> Christoph Lameter wrote:
> >>That's probably due to the fact that we use PageSlab for all pages with 
> >>SLUB
> >>and for bigblock pages in SLOB with this patch. Christoph, Matt, any
> >>suggestions how to fix the PageSlab check in nommu btw?
> >
> >I already posted a patch and commented numerous times. What else needs 
> >to be said? SLUB only uses PageSlab for kmalloc <=4kb.
> 
> (I missed your patch, btw.)
> 
> For SLOB, we will never use ksize() as it doesn't set PageSlab. Is that 
> not a problem?
> 
That means that kobjsize() will hand back PAGE_SIZE for non-compound
pages, which is what it's presently doing anyways. In the case of SLOB
bigblock pages allocated on pass-through to the page allocator, these are
compound pages anyways, so compound_page() should do the right thing
there? That only handles the kmalloc() path though, and not the
kmem_cache_alloc() cases.

Shouldn't SLOB's PageSlab usage should mimic that of SLUB in this case
instead? PG_slab doesn't buy us much if we can already sort out the size
through compound_order(), it's the kmem_cache_alloc() and <= PAGE_SIZE
kmalloc()'s where __GFP_COMP isn't true and where PG_slab should be set.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Pekka Enberg]

Hi Paul,

Paul Mundt wrote:
> Shouldn't SLOB's PageSlab usage should mimic that of SLUB in this case
> instead? PG_slab doesn't buy us much if we can already sort out the size
> through compound_order(), it's the kmem_cache_alloc() and <= PAGE_SIZE
> kmalloc()'s where __GFP_COMP isn't true and where PG_slab should be set.

Well, we should really be calling ksize() in the nommu case and although 
Matt and Christoph don't seem to agree with me here, I'd much rather 
have *all* allocators set PageSlab for all the pages they return (yes, 
including pass-through ones) to get us back where we were with SLAB. We 
currently don't have any means to see whether an arbitrary page is part 
of the slab or not and I'd argue that's PageSlab is an established API 
(that makes sense).

Furthermore, if ksize() in SLOB doesn't work for kmem_cache_alloc() 
pages, I think it should be fixed as well.

			Pekka

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Paul Mundt]

On Thu, May 22, 2008 at 07:43:02AM +0300, Pekka Enberg wrote:
> Paul Mundt wrote:
> >Shouldn't SLOB's PageSlab usage should mimic that of SLUB in this case
> >instead? PG_slab doesn't buy us much if we can already sort out the size
> >through compound_order(), it's the kmem_cache_alloc() and <= PAGE_SIZE
> >kmalloc()'s where __GFP_COMP isn't true and where PG_slab should be set.
> 
> Well, we should really be calling ksize() in the nommu case and although 
> Matt and Christoph don't seem to agree with me here, I'd much rather 
> have *all* allocators set PageSlab for all the pages they return (yes, 
> including pass-through ones) to get us back where we were with SLAB. We 
> currently don't have any means to see whether an arbitrary page is part 
> of the slab or not and I'd argue that's PageSlab is an established API 
> (that makes sense).
> 
> Furthermore, if ksize() in SLOB doesn't work for kmem_cache_alloc() 
> pages, I think it should be fixed as well.
> 
SLOB's kmem_cache_alloc() pages won't have PageSlab set, so ksize()
will never be called, and we will simply always return PAGE_SIZE from
kobjsize() for those objects, as they don't set __GFP_COMP either.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Matt Mackall]

On Thu, 2008-05-22 at 07:43 +0300, Pekka Enberg wrote:
> Hi Paul,
> 
> Paul Mundt wrote:
> > Shouldn't SLOB's PageSlab usage should mimic that of SLUB in this case
> > instead? PG_slab doesn't buy us much if we can already sort out the size
> > through compound_order(), it's the kmem_cache_alloc() and <= PAGE_SIZE
> > kmalloc()'s where __GFP_COMP isn't true and where PG_slab should be set.
> 
> Well, we should really be calling ksize() in the nommu case and although 
> Matt and Christoph don't seem to agree with me here, I'd much rather 
> have *all* allocators set PageSlab for all the pages they return (yes, 
> including pass-through ones) to get us back where we were with SLAB. We 
> currently don't have any means to see whether an arbitrary page is part 
> of the slab or not and I'd argue that's PageSlab is an established API 
> (that makes sense).
> 
> Furthermore, if ksize() in SLOB doesn't work for kmem_cache_alloc() 
> pages, I think it should be fixed as well.

As I've said several times, ksize() on kmem_cache_alloced objects
-cannot work- on SLOB. Calling ksize() on something returned by
kmem_cache_alloc is a categorical error.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Pekka Enberg]

Hi Matt,

On Thu, May 22, 2008 at 6:01 PM, Matt Mackall <mpm@selenic.com> wrote:
> As I've said several times, ksize() on kmem_cache_alloced objects
> -cannot work- on SLOB. Calling ksize() on something returned by
> kmem_cache_alloc is a categorical error.

Well, it's a historical fact that ksize() worked for both kmalloc()
and kmem_cache_alloc() (see the kernedoc comment in mm/slab.c).
However, I think we should just look at getting rid of ksize()
altogether as it's only (ab)used by the nommu code and few call-sites
that open-code krealloc().

                       Pekka

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Christoph Lameter]

On Thu, 22 May 2008, Pekka Enberg wrote:

> Hi Matt,
> 
> On Thu, May 22, 2008 at 6:01 PM, Matt Mackall <mpm@selenic.com> wrote:
> > As I've said several times, ksize() on kmem_cache_alloced objects
> > -cannot work- on SLOB. Calling ksize() on something returned by
> > kmem_cache_alloc is a categorical error.
> 
> Well, it's a historical fact that ksize() worked for both kmalloc()
> and kmem_cache_alloc() (see the kernedoc comment in mm/slab.c).
> However, I think we should just look at getting rid of ksize()
> altogether as it's only (ab)used by the nommu code and few call-sites
> that open-code krealloc().

Good idea. if krealloc is an allocator internal function then it can 
establish the size as needed and we do not have ksize available.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Matt Mackall]

On Thu, 2008-05-22 at 19:13 +0300, Pekka Enberg wrote:
> Hi Matt,
> 
> On Thu, May 22, 2008 at 6:01 PM, Matt Mackall <mpm@selenic.com> wrote:
> > As I've said several times, ksize() on kmem_cache_alloced objects
> > -cannot work- on SLOB. Calling ksize() on something returned by
> > kmem_cache_alloc is a categorical error.
> 
> Well, it's a historical fact that ksize() worked for both kmalloc()
> and kmem_cache_alloc() (see the kernedoc comment in mm/slab.c).

Indeed. It looks like it was in fact introduced for nommu (back in
2.5.47). But much like kfree(kmem_cache_alloc()) is a bogus thing to do,
ksize(kmem_cache_alloc()) is assuming too much about the relationship
between kmalloc and kmem_cache_alloc.

Nommu's accounting code makes two misguided assumptions a) that we can
determine how/whether something was allocated just from a pointer b)
that the size of that object can be determined dynamically in any case
other than kmalloc. But it really shouldn't need to do either of these.

> However, I think we should just look at getting rid of ksize()
> altogether as it's only (ab)used by the nommu code and few call-sites
> that open-code krealloc().

Right.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC/PATCH 1/3] SLAB: Add PageSlab checking to ksize()

[Adrian Bunk]

On Thu, May 22, 2008 at 07:13:56PM +0300, Pekka Enberg wrote:
>...
> However, I think we should just look at getting rid of ksize()
> altogether as it's only (ab)used by the nommu code and few call-sites
> that open-code krealloc().

As soon as your patch for the netfilter usage enters Linus' tree you can 
unexport it which will at least make new abuses from modules impossible.

>                        Pekka

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed