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

[Zhaoyang Huang <huangzhaoyang@gmail.com>]

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!