Re: [PATCH 2/2] x86/numa: Fix the sort compare func used in numa_fill_memblks()

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Jan 12, 2024 at 02:20:04PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > The compare function used to sort memblks into starting address
> > order fails when the result of its u64 address subtraction gets
> > truncated to an int upon return.
> > 
> > The impact of the bad sort is that memblks will be filled out
> > incorrectly. Depending on the set of memblks, a user may see no
> > errors at all but still have a bad fill, or see messages reporting
> > a node overlap that leads to numa init failure:
> > 
> > [] node 0 [mem: ] overlaps with node 1 [mem: ]
> > [] No NUMA configuration found
> > 
> > Replace with a comparison that can only result in: 1, 0, -1.
> 
> Good eye!
> 
> > Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()")
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  arch/x86/mm/numa.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> > index 8ada9bbfad58..65e9a6e391c0 100644
> > --- a/arch/x86/mm/numa.c
> > +++ b/arch/x86/mm/numa.c
> > @@ -934,7 +934,7 @@ static int __init cmp_memblk(const void *a, const void *b)
> >  	const struct numa_memblk *ma = *(const struct numa_memblk **)a;
> >  	const struct numa_memblk *mb = *(const struct numa_memblk **)b;
> >  
> > -	return ma->start - mb->start;
> > +	return (ma->start > mb->start) - (ma->start < mb->start);
> 
> Maybe just do the less clever but obviously correct thing like 
> other unsigned compares like ktime_compare():
> 
>     if (ma->start > mb->start)
>         return 1;
>     if (ma->start < mb->start)
>         return -1;
>     return 0;
> 
> ...but otherwise this looks good to me.

During the upstream march of the original set, I oversimplified and
broke. For this fix, I surveyed the sort compare funcs and lifted this
simple method from commit: 4ac19ead0dfb ("kvm: x86/pmu: Fix the compare
function used by the pmu event filter"). 

Let me see what else I get to trigger a v2.

Thanks for the review!