Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625848

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

Show all headers | View raw

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@kernel.org )
> > > 
> > > 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.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was "zram:  do not use copy_page with non-page alinged address") Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-17 03:50 +0200
  Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Christoph Lameter <cl@linux.com> - 2017-04-17 17:30 +0200
    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 <minchan@kernel.org> - 2017-04-18 02:10 +0200
      Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Michal Hocko <mhocko@kernel.org> - 2017-04-18 09:40 +0200
        Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Michal Hocko <mhocko@kernel.org> - 2017-04-18 13:10 +0200
          Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-19 08:20 +0200
        Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-18 13:10 +0200
        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 <minchan@kernel.org> - 2017-04-19 08:10 +0200
          Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Matthew Wilcox <willy@infradead.org> - 2017-04-19 14:00 +0200
            Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-20 03:50 +0200
              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 <minchan@kernel.org> - 2017-04-20 09:00 +0200
    Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-04-18 12:50 +0200
      Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Christoph Lameter <cl@linux.com> - 2017-04-18 15:30 +0200
    Re: copy_page() on a kmalloc-ed page with DEBUG_SLAB enabled (was  "zram: do not use copy_page with non-page alinged address") Matthew Wilcox <willy@infradead.org> - 2017-04-18 15:20 +0200

csiph-web