Re: [PATCH] arm64/numa: Add more vetting in numa_set_distance()

[Anshuman Khandual]

On 10/29/2018 08:18 PM, Will Deacon wrote:
> On Mon, Oct 29, 2018 at 06:15:42PM +0530, Anshuman Khandual wrote:
>> On 10/29/2018 06:02 PM, John Garry wrote:
>>> On 29/10/2018 12:16, Will Deacon wrote:
>>>> On Mon, Oct 29, 2018 at 12:14:09PM +0000, John Garry wrote:
>>>>> On 29/10/2018 11:25, Will Deacon wrote:
>>>>>> On Fri, Oct 26, 2018 at 09:57:47PM +0800, John Garry wrote:
>>>>>>> Currently it is acceptable to set the distance between 2 separate nodes to
>>>>>>> LOCAL_DISTANCE.
>>>>>>>
>>>>>>> Reject this as it is invalid.
>>>>>>>
>>>>>>> This change avoids a crash reported in [1].
>>>>>>>
>>>>>>> [1] https://www.spinics.net/lists/arm-kernel/msg683304.html
>>>>>>>
>>>>>>> Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
>>>>>>>
>>>>>>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>>>>>>> index 146c04c..6092e3d 100644
>>>>>>> --- a/arch/arm64/mm/numa.c
>>>>>>> +++ b/arch/arm64/mm/numa.c
>>>>>>> @@ -335,7 +335,8 @@ void __init numa_set_distance(int from, int to, int distance)
>>>>>>> ÂÂÂÂ}
>>>>>>>
>>>>>>> ÂÂÂÂif ((u8)distance != distance ||
>>>>>>> -ÂÂÂÂÂÂÂ (from == to && distance != LOCAL_DISTANCE)) {
>>>>>>> +ÂÂÂÂÂÂÂ (from == to && distance != LOCAL_DISTANCE) ||
>>>>>>> +ÂÂÂÂÂÂÂ (from != to && distance == LOCAL_DISTANCE)) {
>>>>>>
>>>>>> The current code here is more-or-less lifted from the x86 implementation
>>>>>> of numa_set_distance().
>>>>>
>>>>> Right, I did notice this. I didn't think that x86 folks would be so
>>>>> concerned since they generally only use ACPI, and the ACPI code already
>>>>> validates these distances in drivers/acpi/numa.c: slit_valid() [unlike OF
>>>>> code].
>>>>>
>>>>> ÂI think we should either factor out the sanity check
>>>>>> into a core helper or make the core code robust to these funny configurations.
>>>>>
>>>>> OK, so to me it would make sense to factor out a sanity check into a core
>>>>> helper.
>>>>
>>>> That, or have the OF code perform the same validation that slit_valid() is
>>>> doing for ACPI. I'm just trying to avoid other architectures running into
>>>> this problem down the line.
>>>>
>>>
>>> Right, OF code should do this validation job if ACPI is doing it
>>> (especially since the DT bindings actually specify the distance rules),
>>> and not rely on the arch NUMA code to accept/reject numa_set_distance()
>>> combinations.
>>
>> I would say this particular condition checking still falls under arch NUMA init
>> code sanity check like other basic tests what numa_set_distance() currently does
>> already but it should not be a necessity for the OF driver to check these. It can
>> choose to check but arch NUMA should check basic things like two different NUMA
>> nodes should not have LOCAL_DISTANCE as distance like in this case.
>>
>> 	(from == to && distance != LOCAL_DISTANCE) ||
>> 		(from != to && distance == LOCAL_DISTANCE))
>>
>>
>>>
>>> And, in addition to this, I'd say OF should disable NUMA if given an
>>> invalid table (like ACPI does).
>>
>> Taking a decision to disable NUMA should be with kernel (arch NUMA) once kernel
>> starts booting. Platform should have sent right values, OF driver trying to
>> adjust stuff what platform has sent with FDT once the kernel starts booting is
>> not right. For example "Kernel NUMA wont like the distance factors lets clean
>> then up before passing on to MM". Disabling NUMA is one such major decision which
>> should be with arch NUMA code not with OF driver.
> 
> I don't fully understand what you're getting at here, but why would the
> check posted by John be arch-specific? It's already done in the core code
> for ACPI, so there's a discrepancy between ACPI and FDT that should be
> resolved. I'd also argue that the subtleties of this check are actually
> based on what the core code is willing to accept in terms of the NUMA
> description, so it's also the best place to enforce it.

Agreed. I had overlooked the existing semantics with respect to ACPI parsing.
Yes, there is a discrepancy with respect to FDT which should be fixed. But
IMHO its also worth to enhance numa_set_distance() checks with this proposed
new check as well.