Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was "zram: do not use copy_page with non-page alinged address")

[Minchan Kim]

Hello Michal,

On Tue, Apr 18, 2017 at 09:33:07AM +0200, Michal Hocko wrote:
> On Tue 18-04-17 09:03:19, Minchan Kim wrote:
> > On Mon, Apr 17, 2017 at 10:20:42AM -0500, Christoph Lameter wrote:
> > > On Mon, 17 Apr 2017, Sergey Senozhatsky wrote:
> > > 
> > > > Minchan reported that doing copy_page() on a kmalloc(PAGE_SIZE) page
> > > > with DEBUG_SLAB enabled can cause a memory corruption (See below or
> > > > lkml.kernel.org/r/1492042622-12074-2-git-send-email-minchan@xxxxxxxxxx )
> > > 
> > > Yes the alignment guarantees do not require alignment on a page boundary.
> > > 
> > > The alignment for kmalloc allocations is controlled by KMALLOC_MIN_ALIGN.
> > > Usually this is either double word aligned or cache line aligned.
> > > 
> > > > that's an interesting problem. arm64 copy_page(), for instance, wants src
> > > > and dst to be page aligned, which is reasonable, while generic copy_page(),
> > > > on the contrary, simply does memcpy(). there are, probably, other callpaths
> > > > that do copy_page() on kmalloc-ed pages and I'm wondering if there is some
> > > > sort of a generic fix to the problem.
> > > 
> > > Simple solution is to not allocate pages via the slab allocator but use
> > > the page allocator for this. The page allocator provides proper alignment.
> > > 
> > > There is a reason it is called the page allocator because if you want a
> > > page you use the proper allocator for it.
> 
> Agreed. Using the slab allocator for page sized object is just wasting
> cycles and additional metadata.
> 
> > It would be better if the APIs works with struct page, not address but
> > I can imagine there are many cases where don't have struct page itself
> > and redundant for kmap/kunmap.
> 
> I do not follow. Why would you need kmap for something that is already
> in the kernel space?

Because it can work with highmem pages.

> 
> > Another approach is the API does normal thing for non-aligned prefix and
> > tail space and fast thing for aligned space.
> > Otherwise, it would be happy if the API has WARN_ON non-page SIZE aligned
> > address.
> 
> copy_page is a performance sensitive function and I believe that we do
> those tricks exactly for this purpose. Why would we want to add an
> overhead for the alignment check or WARN_ON when using unaligned
> pointers? I do see that debugging a subtle memory corruption is PITA
> but that doesn't imply we should clobber the hot path IMHO.

What I wanted is VM_WARN_ON so it shouldn't be no overhead for whom
want really fast kernel. 

> 
> A big fat warning for copy_page would be definitely helpful though.

It's better than as-is but everyone doesn't read comment like such
simple API(e.g., clear_page(void *mem)), esp. And once it happens,
it's really subtle because for exmaple, you have not seen any bug
without slub debug. Based on it, you add new feature and crashed
for testing. To find a bug, you enable slub_debug. Bang.
you encounter a new bug lurked for a long time.
VM_WARN_ON would be valuable but I'm okay any option which might
have better to catch the bug if someone donates his time to fix
it up.

Thanks.