Re: [PATCH v1] mm/page_alloc: clear pages in alloc_contig_pages() with init_on_alloc=1 or __GFP_ZERO

[Michal Hocko]

On Wed 11-11-20 11:05:21, David Hildenbrand wrote:
> On 11.11.20 10:58, Vlastimil Babka wrote:
> > On 11/11/20 10:06 AM, David Hildenbrand wrote:
> > > On 11.11.20 09:47, Michal Hocko wrote:
> > > > On Tue 10-11-20 20:32:40, David Hildenbrand wrote:
> > > > > commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and
> > > > > init_on_free=1 boot options") resulted with init_on_alloc=1 in all pages
> > > > > leaving the buddy via alloc_pages() and friends to be
> > > > > initialized/cleared/zeroed on allocation.
> > > > > 
> > > > > However, the same logic is currently not applied to
> > > > > alloc_contig_pages(): allocated pages leaving the buddy aren't cleared
> > > > > with init_on_alloc=1 and init_on_free=0. Let's also properly clear
> > > > > pages on that allocation path and add support for __GFP_ZERO.
> > > > 
> > > > AFAIR we do not have any user for __GFP_ZERO right? Not that this is
> > > 
> > > Sorry, I had extended information under "---" but accidentally
> > > regenerated the patch before sending it out.
> > > 
> > > __GFP_ZERO is not used yet. It's intended to be used in
> > > https://lkml.kernel.org/r/20201029162718.29910-1-david@xxxxxxxxxx
> > > and I can move that change into a separate patch if desired.

OK, it would make sense to add it with its user.

> > > > harmful but it is better to call that explicitly because a missing
> > > > implementation would be a real problem and as such a bug fix.
> > > > 
> > > > I am also not sure handling init_on_free at the higher level is good.
> > > > As we have discussed recently the primary point of this feature is to
> > > > add clearing at very few well defined entry points rather than spill it over
> > > > many places. In this case the entry point for the allocator is
> > > > __isolate_free_page which removes pages from the page allocator. I
> > > > haven't checked how much this is used elsewhere but I would expect
> > > > init_on_alloc to be handled there.
> > > 
> > > Well, this is the entry point to our range allocator, which lives in
> > > page_alloc.c - used by actual high-level allocators (CMA, gigantic
> > > pages, etc). It's just a matter of taste where we want to have that
> > > handling exactly inside our allocator.

Yes I completely agree here. I just believe it should the lowest we can
achieve.

> > I agree alloc_contig_range() is fine as an entry point.
> 
> Thanks, let's see if Michal insists of having this somewhere inside
> isolate_freepages_range() instead.
 
It's not that I would be insisting. I am just pointing out that changes
like this one go against the idea of init_on_alloc because it is adding
more special casing and long term more places to be really careful about
when one has to be really careful to not undermine the security aspect
of the feature. I haven't really checked why compaction is not the
problem but I suspect it is the fact that it unconditionally copy the
full page content to the isolated page so there is no way to sneak
any data leak there. That is fine. We should however make that clear by
using a special cased function which skips this particular
initialization and make sure everybody else will just do the right thing
without much thinking.

-- 
Michal Hocko
SUSE Labs