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

[Russell King (Oracle)]

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 at infradead.org> wrote:
> >
> > On Tue, Sep 12, 2023 at 06:33:34PM +0800, zhaoyang.huang wrote:
> > > From: Zhaoyang Huang <zhaoyang.huang at 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!