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

[John Garry]

> > Agreed. slit_valid() on the ACPI parsing is currently enforcing that before
> > acpi_numa_slit_init() which would call into numa_set_distance(). Hence arch
> > NUMA code numa_set_distance() never had the opportunity to do the sanity
> > checks as ACPI slit_valid() has completely invalidated the table.
> >
> > Unlike ACPI path, of_numa_parse_distance_map_v1() does not do any sanity
> > checks on the distance values parse from the "distance-matrix" property
> > and all the checks directly falls on numa_set_distance(). This needs to
> > be fixed in line with ACPI
> >
> > * If (to == from) ---> distance = LOCAL_DISTANCE
> > * If (to != from) ---> distance > LOCAL_DISTANCE
> >
> > At the same time its okay to just enhance numa_set_distance() test coverage
> > to include this new test. If we would have trusted firmware parsing all the
> > way, existing basic checks about node range, distance stuff should not have
> > been there in numa_set_distance(). Hence IMHO even if we fix the OF driver
> > part, we should include this new check there as well.
>
> I don't see what we gain by duplicating the check. In fact, it has a few
> downsides:
>
>   (1) It confuses the semantics of the API, because it is no longer clear
>       who "owns" the check
>
>   (2) It duplicates code in each architecture
>
>   (3) Some clever-cloggs will remove at least some of the duplication in
>       future
>
> I'm not willing to accept the check in the arm64 code if we update the
> OF code.
>
> I think the way forward here is for John to fix the crash he reported by
> adding the check to the OF code.

I was planning on doing that.

If somebody wants to follow up with
> subsequent patches to move more of the checking out of the arch code, then
> we can review that as a separate series.

Cheers,
John

> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> .