Re: [PATCH] arch: arm: remove redundant clear_page when CONFIG_INIT_ON_ALLOC_DEFAULT_ON is on

[Zhaoyang Huang]

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@xxxxxxxxx> wrote:
>
> On Wed, Sep 13, 2023 at 4:17 PM Russell King (Oracle)
> <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > > > > From: Zhaoyang Huang <zhaoyang.huang@xxxxxxxxxx>
> > > > >
> > > > > 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!