Re: [PATCH v5 2/3] arm64: mmu: avoid allocating pages while splitting the linear mapping

[Yeoreum Yun <yeoreum.yun@arm.com>]

> > > My concern is that if a secondary CPU can race and cause a split, that is
> > > unsound because we have determined that although the primary CPU supports BBML2,
> > > at least one of the secondary CPUs does not. So splitting a live mapping is unsafe.
> > >
> > > I just had a brief chat with Rutland, and he agrees that this _could_ be a
> > > problem. Basically there is a window between onlining the secondary cpus and
> > > entering the stop_machine() where one of those cpus _could_ end up doing
> > > something that causes us to split the linear map.
>
> If I remember correctly, split_kernel_leaf_mapping() does call
> system_supports_bbml2_noabort() before doing real split. So we basically
> should fall into two categories:
>
> 1. bbml2_noabort is supported on all cpus. Everything is fine.
> 2. bbml2_noabort is not supported on all cpus. split_kernel_leaf_mapping()
> just returns 0. Kernel doesn't split page table, so there won't be TLB
> conflict issue. But the following page prot update may see unexpected block
> mapping, then a   WARN  will be raised and it will return -EINVAL. So the
> worst case is the caller will fail (IIRC all the callers of set_memory_*()
> handle the failure), and we can know who is trying to change linear mapping
> before the linear mapping gets                    finalized. AFAICT I
> haven't seen such WARN yet.

Thanks for the great detail :)
I've missed system_supports_bbml2_noabort() in split_kernel_leaf_mapping().

>
> > >
> > > I'm not immediately sure how to solve that.
>
> Do we need some synchronization mechanism? If the linear mapping is not
> finalized yet, split_kernel_leaf_mapping() will spin. For example, something
> like this off the top of my head,
>
> DEFINE_STATIC_KEY_FALSE(linear_mapping_finalized);
>
> Once the linear mapping is finalized, we can call
> static_branch_enable(&linear_mapping_finalized);
>
> In split_kernel_leaf_mapping(), we can just do:
>
> retry:
>     if (!static_branch_likely(&linear_mapping_finalized))
>         goto retry;
>
>
> There may be better way to handle it. But this case should be very unlikely
> IMHO. It sounds crazy to have such complicated kernel threads run so early.
> I'm not sure whether we should pay immediate attention to it or not.

Thinking about it again, I’m not sure whether
it is acceptable to use a sleepable synchronization mechanism at this stage,
like split_kernel_leaf_mapping() does with mutex_lock()
(even though it may be technically possible).
It also feels odd that this function can be called at this point in time.

If this is indeed considered problematic,
I think it would be better to simply return -EINVAL immediately
when linear_mapping_finalized has not yet been completed.

--
Sincerely,
Yeoreum Yun