arch: arm: remove redundant clear_page when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on

  From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>

Double times of clear_page observed in an arm SOC(A55) when
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
vma_alloc_zeroed_movable_folio within do_anonymous_pages.
Since there is no D-cache operation within v6's clear_user_highpage,
I would like to suggest to remove the redundant clear_page.

struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
				   unsigned long vaddr)
{
	struct folio *folio;

//first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
	if (folio)
//second clear_page which is meaningless since it do nothing to D-cache in armv6
		clear_user_highpage(&folio->page, vaddr);

	return folio;
}

Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
---
 arch/arm/mm/copypage-v6.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> 
> Double times of clear_page observed in an arm SOC(A55) when
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
> vma_alloc_zeroed_movable_folio within do_anonymous_pages.
> Since there is no D-cache operation within v6's clear_user_highpage,
> I would like to suggest to remove the redundant clear_page.
> 
> struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> 				   unsigned long vaddr)
> {
> 	struct folio *folio;
> 
> //first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
> 	folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> 	if (folio)
> //second clear_page which is meaningless since it do nothing to D-cache in armv6
> 		clear_user_highpage(&folio->page, vaddr);

This is, of course, not the only place which calls clear_user_highpage().
Please explain why this patch is safe for all the _other_ places which
call clear_user_highpage().

> 	return folio;
> }
> 
> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> ---
>  arch/arm/mm/copypage-v6.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/mm/copypage-v6.c b/arch/arm/mm/copypage-v6.c
> index a1a71f36d850..6f8bee1b3203 100644
> --- a/arch/arm/mm/copypage-v6.c
> +++ b/arch/arm/mm/copypage-v6.c
> @@ -9,6 +9,7 @@
>  #include <linux/mm.h>
>  #include <linux/highmem.h>
>  #include <linux/pagemap.h>
> +#include <linux/gfp.h>
>  
>  #include <asm/shmparam.h>
>  #include <asm/tlbflush.h>
> @@ -45,6 +46,13 @@ static void v6_copy_user_highpage_nonaliasing(struct page *to,
>   */
>  static void v6_clear_user_highpage_nonaliasing(struct page *page, unsigned long vaddr)
>  {
> +	/*
> +	 * This criteria only help bailing out when CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> +	 * is on. The page has been memset to zero when it allocated and the
> +	 * bellowing clear_page will do it again.
> +	 */
> +	if (want_init_on_alloc(GFP_HIGHUSER_MOVABLE))
> +		return;
>  	void *kaddr = kmap_atomic(page);
>  	clear_page(kaddr);
>  	kunmap_atomic(kaddr);
> -- 
> 2.25.1
>
  

On Tue, Sep 12, 2023 at 8:18 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> >
> > Double times of clear_page observed in an arm SOC(A55) when
> > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
> > vma_alloc_zeroed_movable_folio within do_anonymous_pages.
> > Since there is no D-cache operation within v6's clear_user_highpage,
> > I would like to suggest to remove the redundant clear_page.
> >
> > struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> >                                  unsigned long vaddr)
> > {
> >       struct folio *folio;
> >
> > //first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
> >       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> >       if (folio)
> > //second clear_page which is meaningless since it do nothing to D-cache in armv6
> >               clear_user_highpage(&folio->page, vaddr);
>
> This is, of course, not the only place which calls clear_user_highpage().
> Please explain why this patch is safe for all the _other_ places which
> call clear_user_highpage().
Here are all positions called clear_user_highpage which are paired
with alloc_pages. IMO,  it is safe to skip the second clear_page under
armv6.

drivers/media/v4l2-core/videobuf-dma-sg.c:441:
clear_user_highpage(page, vmf->address);
fs/dax.c:1612: clear_user_highpage(vmf->cow_page, vmf->address);
include/linux/highmem.h:231: clear_user_highpage(&folio->page, vaddr);
mm/memory.c:5974: clear_user_highpage(p, addr + i * PAGE_SIZE);
mm/memory.c:5982: clear_user_highpage(page + idx, addr);
mm/shmem.c:2621: clear_user_highpage(&folio->page, dst_addr);
mm/khugepaged.c:796: clear_user_highpage(page, _address);

>
> >       return folio;
> > }
> >
> > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > ---
> >  arch/arm/mm/copypage-v6.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/arm/mm/copypage-v6.c b/arch/arm/mm/copypage-v6.c
> > index a1a71f36d850..6f8bee1b3203 100644
> > --- a/arch/arm/mm/copypage-v6.c
> > +++ b/arch/arm/mm/copypage-v6.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/highmem.h>
> >  #include <linux/pagemap.h>
> > +#include <linux/gfp.h>
> >
> >  #include <asm/shmparam.h>
> >  #include <asm/tlbflush.h>
> > @@ -45,6 +46,13 @@ static void v6_copy_user_highpage_nonaliasing(struct page *to,
> >   */
> >  static void v6_clear_user_highpage_nonaliasing(struct page *page, unsigned long vaddr)
> >  {
> > +     /*
> > +      * This criteria only help bailing out when CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> > +      * is on. The page has been memset to zero when it allocated and the
> > +      * bellowing clear_page will do it again.
> > +      */
> > +     if (want_init_on_alloc(GFP_HIGHUSER_MOVABLE))
> > +             return;
> >       void *kaddr = kmap_atomic(page);
> >       clear_page(kaddr);
> >       kunmap_atomic(kaddr);
> > --
> > 2.25.1
> >
  

On Wed, Sep 13, 2023 at 09:13:14AM +0800, Zhaoyang Huang wrote:
> On Tue, Sep 12, 2023 at 8:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > >
> > > Double times of clear_page observed in an arm SOC(A55) when
> > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
> > > vma_alloc_zeroed_movable_folio within do_anonymous_pages.
> > > Since there is no D-cache operation within v6's clear_user_highpage,
> > > I would like to suggest to remove the redundant clear_page.

So if CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, then what ensures
that the page is cleared?

> > >
> > > struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> > >                                  unsigned long vaddr)
> > > {
> > >       struct folio *folio;
> > >
> > > //first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
> > >       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> > >       if (folio)
> > > //second clear_page which is meaningless since it do nothing to D-cache in armv6
> > >               clear_user_highpage(&folio->page, vaddr);

If this clear_user_highpage() is removed, how is this code then safe when
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled?

> >
> > This is, of course, not the only place which calls clear_user_highpage().
> > Please explain why this patch is safe for all the _other_ places which
> > call clear_user_highpage().
> Here are all positions called clear_user_highpage which are paired
> with alloc_pages. IMO,  it is safe to skip the second clear_page under
> armv6.

No.

Looking at, for example, the v4l case... This allocates a page and
provides it to userspace. The page is allocated using GFP_USER |
__GFP_DMA32. This does not set __GFP_ZERO. If
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, the page will not
be initialised, and thus we will leak any data in that page to
userspace.

Now, it's not just about whether that configuration symbol is enabled
in the kernel configuration - there is a command line argument to
consider as well. CONFIG_INIT_ON_ALLOC_DEFAULT_ON can be y, but with
init_on_alloc=0 passed to the kernel, if we remove the above
clear_user_highpage(), the kernel then becomes unsafe.

However, it's more than that. The kernel allocator has no idea that the
page will be mapped to userspace, so it can't do the "clear the page at
the user cache colour" trick for VIPT aliasing caches, which ensures
that we hit cache lines that the user will see. So, I think we would
then have to add arch specific cache operations to write-back the
zeroing of the kernel mapping, _and_ cache operations to discard any
data in the user cache colour.

So, essentially, I don't think that _even_ when init_on_alloc is
enabled, we can skip calling clear_user_highpage() as that would lead
to data exposure to userspace.
  

On Wed, Sep 13, 2023 at 4:17 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Sep 13, 2023 at 09:13:14AM +0800, Zhaoyang Huang wrote:
> > On Tue, Sep 12, 2023 at 8:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > >
> > > > Double times of clear_page observed in an arm SOC(A55) when
> > > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
> > > > vma_alloc_zeroed_movable_folio within do_anonymous_pages.
> > > > Since there is no D-cache operation within v6's clear_user_highpage,
> > > > I would like to suggest to remove the redundant clear_page.
>
> So if CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, then what ensures
> that the page is cleared?

>
> > > >
> > > > struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> > > >                                  unsigned long vaddr)
> > > > {
> > > >       struct folio *folio;
> > > >
> > > > //first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
> > > >       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> > > >       if (folio)
> > > > //second clear_page which is meaningless since it do nothing to D-cache in armv6
> > > >               clear_user_highpage(&folio->page, vaddr);
>
> If this clear_user_highpage() is removed, how is this code then safe when
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled?
when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is off, want_init_on_alloc() will
return false and then clear_user_highpage will be called
>
> > >
> > > This is, of course, not the only place which calls clear_user_highpage().
> > > Please explain why this patch is safe for all the _other_ places which
> > > call clear_user_highpage().
> > Here are all positions called clear_user_highpage which are paired
> > with alloc_pages. IMO,  it is safe to skip the second clear_page under
> > armv6.
>
> No.
>
> Looking at, for example, the v4l case... This allocates a page and
> provides it to userspace. The page is allocated using GFP_USER |
> __GFP_DMA32. This does not set __GFP_ZERO. If
> CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, the page will not
> be initialised, and thus we will leak any data in that page to
> userspace.
as explained above, clear_user_highpage will be called in this scenario
>
> Now, it's not just about whether that configuration symbol is enabled
> in the kernel configuration - there is a command line argument to
> consider as well. CONFIG_INIT_ON_ALLOC_DEFAULT_ON can be y, but with
> init_on_alloc=0 passed to the kernel, if we remove the above
> clear_user_highpage(), the kernel then becomes unsafe.
Both of CONFIG_INIT_ON_ALLOC_DEFAULT_ON and cmdline configuration take
effect via the global variable init_on_alloc which is judged within
want_init_on_alloc()
>
> However, it's more than that. The kernel allocator has no idea that the
> page will be mapped to userspace, so it can't do the "clear the page at
> the user cache colour" trick for VIPT aliasing caches, which ensures
> that we hit cache lines that the user will see. So, I think we would
> then have to add arch specific cache operations to write-back the
> zeroing of the kernel mapping, _and_ cache operations to discard any
> data in the user cache colour.
ok, do you mean you will update v6's clear_user_highpage from memset
to D-cache flush things?
>
> So, essentially, I don't think that _even_ when init_on_alloc is
> enabled, we can skip calling clear_user_highpage() as that would lead
> to data exposure to userspace.
This patch only suggests making changes on the specific v6
architecture where clear_user_highpage equal to clear_page so far.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
  

any further comments, it is arised from a real performance issue
observed double times of memset in ARM A55 which should be waste. What
this patch suggest is to remove the latter one while ensure the page
will be cleared under all scenarios

On Wed, Sep 13, 2023 at 4:53 PM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
>
> On Wed, Sep 13, 2023 at 4:17 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Sep 13, 2023 at 09:13:14AM +0800, Zhaoyang Huang wrote:
> > > On Tue, Sep 12, 2023 at 8:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > > >
> > > > > Double times of clear_page observed in an arm SOC(A55) when
> > > > > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on, which introduced by
> > > > > vma_alloc_zeroed_movable_folio within do_anonymous_pages.
> > > > > Since there is no D-cache operation within v6's clear_user_highpage,
> > > > > I would like to suggest to remove the redundant clear_page.
> >
> > So if CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, then what ensures
> > that the page is cleared?
>
> >
> > > > >
> > > > > struct folio *vma_alloc_zeroed_movable_folio(struct vm_area_struct *vma,
> > > > >                                  unsigned long vaddr)
> > > > > {
> > > > >       struct folio *folio;
> > > > >
> > > > > //first clear_page invoked by vma_alloc_folio==>alloc_page==>post_alloc_hook
> > > > >       folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0, vma, vaddr, false);
> > > > >       if (folio)
> > > > > //second clear_page which is meaningless since it do nothing to D-cache in armv6
> > > > >               clear_user_highpage(&folio->page, vaddr);
> >
> > If this clear_user_highpage() is removed, how is this code then safe when
> > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled?
> when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is off, want_init_on_alloc() will
> return false and then clear_user_highpage will be called
> >
> > > >
> > > > This is, of course, not the only place which calls clear_user_highpage().
> > > > Please explain why this patch is safe for all the _other_ places which
> > > > call clear_user_highpage().
> > > Here are all positions called clear_user_highpage which are paired
> > > with alloc_pages. IMO,  it is safe to skip the second clear_page under
> > > armv6.
> >
> > No.
> >
> > Looking at, for example, the v4l case... This allocates a page and
> > provides it to userspace. The page is allocated using GFP_USER |
> > __GFP_DMA32. This does not set __GFP_ZERO. If
> > CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not enabled, the page will not
> > be initialised, and thus we will leak any data in that page to
> > userspace.
> as explained above, clear_user_highpage will be called in this scenario
> >
> > Now, it's not just about whether that configuration symbol is enabled
> > in the kernel configuration - there is a command line argument to
> > consider as well. CONFIG_INIT_ON_ALLOC_DEFAULT_ON can be y, but with
> > init_on_alloc=0 passed to the kernel, if we remove the above
> > clear_user_highpage(), the kernel then becomes unsafe.
> Both of CONFIG_INIT_ON_ALLOC_DEFAULT_ON and cmdline configuration take
> effect via the global variable init_on_alloc which is judged within
> want_init_on_alloc()
> >
> > However, it's more than that. The kernel allocator has no idea that the
> > page will be mapped to userspace, so it can't do the "clear the page at
> > the user cache colour" trick for VIPT aliasing caches, which ensures
> > that we hit cache lines that the user will see. So, I think we would
> > then have to add arch specific cache operations to write-back the
> > zeroing of the kernel mapping, _and_ cache operations to discard any
> > data in the user cache colour.
> ok, do you mean you will update v6's clear_user_highpage from memset
> to D-cache flush things?
> >
> > So, essentially, I don't think that _even_ when init_on_alloc is
> > enabled, we can skip calling clear_user_highpage() as that would lead
> > to data exposure to userspace.
> This patch only suggests making changes on the specific v6
> architecture where clear_user_highpage equal to clear_page so far.
> >
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!