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

[Dan Williams <dan.j.williams@intel.com>]

Alison Schofield wrote:
> 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.

If nothing else comes in, then don't spin just for this.