Re: [PATCH mm-unstable v1] mm/hugetlb_vmemmap: fix race with speculative PFN walkers

[Yu Zhao]

On Thu, Jun 27, 2024 at 1:25 AM Muchun Song <muchun.song@xxxxxxxxx> wrote:
>
>
>
> On 2024/6/27 12:43, Yu Zhao wrote:
> > While investigating HVO for THPs [1], it turns out that speculative
> > PFN walkers like compaction can race with vmemmap modifications, e.g.,
> >
> >    CPU 1 (vmemmap modifier)         CPU 2 (speculative PFN walker)
> >    -------------------------------  ------------------------------
> >    Allocates an LRU folio page1
> >                                     Sees page1
> >    Frees page1
> >
> >    Allocates a hugeTLB folio page2
> >    (page1 being a tail of page2)
> >
> >    Updates vmemmap mapping page1
> >                                     get_page_unless_zero(page1)
> >
> > Even though page1->_refcount is zero after HVO, get_page_unless_zero()
> > can still try to modify this read-only field, resulting in a crash.
> >
> > An independent report [2] confirmed this race.
> >
> > There are two discussed approaches to fix this race:
> > 1. Make RO vmemmap RW so that get_page_unless_zero() can fail without
> >     triggering a PF.
> > 2. Use RCU to make sure get_page_unless_zero() either sees zero
> >     page->_refcount through the old vmemmap or non-zero page->_refcount
> >     through the new one.
> >
> > The second approach is preferred here because:
> > 1. It can prevent illegal modifications to struct page[] that has been
> >     HVO'ed;
> > 2. It can be generalized, in a way similar to ZERO_PAGE(), to fix
> >     similar races in other places, e.g., arch_remove_memory() on x86
> >     [3], which frees vmemmap mapping offlined struct page[].
> >
> > While adding synchronize_rcu(), the goal is to be surgical, rather
> > than optimized. Specifically, calls to synchronize_rcu() on the error
> > handling paths can be coalesced, but it is not done for the sake of
> > Simplicity: noticeably, this fix removes ~50% more lines than it adds.
>
> I suggest adding some user-visible effect here like for use
> case of nr_overcommit_hugepages, synchronize_rcu() will make
> this use case worse.
>
> >
> > [1] https://lore.kernel.org/20240229183436.4110845-4-yuzhao@xxxxxxxxxx/
> > [2] https://lore.kernel.org/917FFC7F-0615-44DD-90EE-9F85F8EA9974@xxxxxxxxx/
> > [3] https://lore.kernel.org/be130a96-a27e-4240-ad78-776802f57cad@xxxxxxxxxx/
> >
> > Signed-off-by: Yu Zhao <yuzhao@xxxxxxxxxx>
>
> Acked-by: Muchun Song <muchun.song@xxxxxxxxx>
>
> A nit below.

Thanks for reviewing!

I've addressed all your suggestions in v2.