Re: Oops in a driver while using SLUB as a SLAB allocator

[Hugh Dickins <hugh@veritas.com>]

On Fri, 22 Jun 2007, Christoph Lameter wrote:
> On Fri, 22 Jun 2007, Hugh Dickins wrote:
> 
> > I'm quite happy with this approach for 2.6.23-rc, along with your ARM
> > dma_map patch which (if I understood aright) rmk approved.  Except,
> > haven't you misplaced that VM_BUG_ON between an if and its else?
> 
> Right.
> 
> > And I'd much prefer you to make it an outright BUG_ON, because
> > many testers have those VM_BUG_ONs configured out.
> 
> Thought about it but doing so would create quite a load of BUG_ONs in the 
> VM given the frequent use of that particular inline function. And AFAIK
> we are only aware of one other potential call site that could cause 
> trouble. Many arches have run SLUB now for awhile and would certainly have 
> shown issues if they would do strange things with slab allocs. Even with 
> SLAB they would have to be very careful in order to make this work. So I 
> think its rather unlikely that this is going to be triggered. Its 
> primarily useful for debugging if strange things start to happen.
> The VM_BUG_ON could stay there for good to make sure development does not
> result in similar issues in the future.

Okay; and I was overlooking that (as in this case) we'd probably get
an easily debuggable oops instead of the explicit BUG when it is
configured out.

> 
> Fixed up patch:
> 
> 
> 
> 
> Add VM_BUG_ON in case someone uses page_mapping on a slab page
> 
> Detect slab objects being passed to the page oriented functions of the VM.
> 
> It is not sufficient to simply return NULL because the functions calling
> page_mapping may depend on other items of the page_struct also to be setup
> properly. Moreover slab object may not be properly aligned. The page oriented
> functions of the VM expect to operate on page aligned, page sized objects.
> Operations on object straddling page boundaries may only affect the objects
> partially which may lead to surprising results.
> 
> It is better to detect eventually remaining uses and eliminate them.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>

Acked-by: Hugh Dickins <hugh@veritas.com>
(immediately after 2.6.22, accompanied by your ARM patch)

> 
> ---
>  include/linux/mm.h |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> Index: linux-2.6/include/linux/mm.h
> ===================================================================
> --- linux-2.6.orig/include/linux/mm.h	2007-06-22 10:33:27.000000000 -0700
> +++ linux-2.6/include/linux/mm.h	2007-06-22 11:44:10.000000000 -0700
> @@ -601,12 +601,9 @@ static inline struct address_space *page
>  {
>  	struct address_space *mapping = page->mapping;
>  
> +	VM_BUG_ON(PageSlab(page));
>  	if (unlikely(PageSwapCache(page)))
>  		mapping = &swapper_space;
> -#ifdef CONFIG_SLUB
> -	else if (unlikely(PageSlab(page)))
> -		mapping = NULL;
> -#endif
>  	else if (unlikely((unsigned long)mapping & PAGE_MAPPING_ANON))
>  		mapping = NULL;
>  	return mapping;
>