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

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]

Hello,

I'll fork it into a separate thread and Cc more MM people.
sorry for top-posting.

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 )

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.

> > On (04/13/17 09:17), Minchan Kim wrote:
> > > The copy_page is optimized memcpy for page-alinged address.
> > > If it is used with non-page aligned address, it can corrupt memory which
> > > means system corruption. With zram, it can happen with
> > > 
> > > 1. 64K architecture
> > > 2. partial IO
> > > 3. slub debug
> > > 
> > > Partial IO need to allocate a page and zram allocates it via kmalloc.
> > > With slub debug, kmalloc(PAGE_SIZE) doesn't return page-size aligned
> > > address. And finally, copy_page(mem, cmem) corrupts memory.
> > 
> > which would be the case for many other copy_page() calls in the kernel.
> > right? if so - should the fix be in copy_page() then?
> 
> I thought about it but was not sure it's good idea by several reasons
> (but don't want to discuss it in this thread).
> 
> Anyway, it's stable stuff so I don't want to make the patch bloat.
> If you believe it is right direction and valuable, you could be
> a volunteer. :)

	-ss

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]

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.

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]

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.

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.

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.

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]

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?

> 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.

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

-- 
Michal Hocko
SUSE Labs

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]

On (04/18/17 09:33), Michal Hocko wrote:
[..]
> > 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.

a wild thought,

use
	#define copy_page(to,from)	memcpy((to), (from), PAGE_SIZE)

when DEBUG_SLAB is set? so arch copy_page() (if provided by arch)
won't be affected otherwise.

	-ss

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]

On Tue 18-04-17 19:56:41, Sergey Senozhatsky wrote:
> On (04/18/17 09:33), Michal Hocko wrote:
> [..]
> > > 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.
> 
> a wild thought,
> 
> use
> 	#define copy_page(to,from)	memcpy((to), (from), PAGE_SIZE)
> 
> when DEBUG_SLAB is set? so arch copy_page() (if provided by arch)
> won't be affected otherwise.

Wouldn't this just paper over bugs? SLAB is not guaranteed to provide
page size aligned object AFAIR.
-- 
Michal Hocko
SUSE Labs

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]

On (04/18/17 13:06), Michal Hocko wrote:
[..]
> > > copy_page is a performance sensitive function and I believe that we do
> > > those tricks exactly for this purpose.
> > 
> > a wild thought,
> > 
> > use
> > 	#define copy_page(to,from)	memcpy((to), (from), PAGE_SIZE)
> > 
> > when DEBUG_SLAB is set? so arch copy_page() (if provided by arch)
> > won't be affected otherwise.
> 
> SLAB is not guaranteed to provide page size aligned object AFAIR.

oh, if there are no guarantees for page_sized allocations regardless
the .config then agree, won't help.

	-ss

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@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.

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]

On Wed, Apr 19, 2017 at 03:02:37PM +0900, Minchan Kim wrote:
> On Tue, Apr 18, 2017 at 09:33:07AM +0200, Michal Hocko wrote:
> > 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.

That's copy_user_highpage().  If you want to define a new arch API
copy_highpage(), feel free to make a case for it ...

> > > 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.

Why not just use memcpy()?  Is copy_page() significantly faster than
memcpy() for a PAGE_SIZE amount of data?

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]

On (04/19/17 04:51), Matthew Wilcox wrote:
[..]
> > > > 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.
> 
> Why not just use memcpy()?  Is copy_page() significantly faster than
> memcpy() for a PAGE_SIZE amount of data?

that's a good point.

I was going to ask yesterday - do we even need copy_page()? arch that
provides well optimized copy_page() quite likely provides somewhat
equally optimized memcpy(). so may be copy_page() is not even needed?

	-ss

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]

On Thu, Apr 20, 2017 at 10:45:42AM +0900, Sergey Senozhatsky wrote:
> On (04/19/17 04:51), Matthew Wilcox wrote:
> [..]
> > > > > 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.
> > 
> > Why not just use memcpy()?  Is copy_page() significantly faster than
> > memcpy() for a PAGE_SIZE amount of data?
> 
> that's a good point.
> 
> I was going to ask yesterday - do we even need copy_page()? arch that
> provides well optimized copy_page() quite likely provides somewhat
> equally optimized memcpy(). so may be copy_page() is not even needed?

I don't know.

Just I found https://download.samba.org/pub/paulus/ols-2003-presentation.pdf
and heard https://lkml.org/lkml/2017/4/10/1270.

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]

On (04/17/17 10:20), 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.

sure, but at the same time it's not completely uncommon and unseen thing

~/_next$ git grep kmalloc | grep PAGE_SIZE | wc -l
75

not all, if any, of those pages get into copy_page(), of course. may be... hopefully.
so may be a warning would make sense and save time some day. but up to MM
people to decide.


p.s. Christoph, FYI, gmail automatically marked your message
     as a spam message, for some reason.

	-ss

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]

On Tue, 18 Apr 2017, Sergey Senozhatsky wrote:

> > Simple solution is to not allocate pages via the slab allocator but use
> > the page allocator for this. The page allocator provides proper alignment.
>
> sure, but at the same time it's not completely uncommon and unseen thing
>
> ~/_next$ git grep kmalloc | grep PAGE_SIZE | wc -l
> 75

Of course if you want a PAGE_SIZE object that is not really page aligned
etc then its definitely ok to use.

> not all, if any, of those pages get into copy_page(), of course. may be... hopefully.
> so may be a warning would make sense and save time some day. but up to MM
> people to decide.

Slab objects are copied using memcpy. copy_page is for pages aligned to
page boundaries and the arch code there may have additional expectations
that cannot be met by the slab allocators.

> p.s. Christoph, FYI, gmail automatically marked your message
>      as a spam message, for some reason.

Weird. Any more details as to why?

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]

On Mon, Apr 17, 2017 at 10:20:42AM -0500, Christoph Lameter wrote:
> 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.

Previous discussion on this topic:

https://lwn.net/Articles/669015/
https://lwn.net/Articles/669020/