Race condition in build_all_zonelists() when offlining movable zone

[Mel Gorman]

On Tue, Aug 23, 2022 at 02:18:27PM +0200, Michal Hocko wrote:
> On Tue 23-08-22 12:09:46, Mel Gorman wrote:
> > On Tue, Aug 23, 2022 at 12:34:09PM +0200, David Hildenbrand wrote:
> > > > @@ -6553,7 +6576,7 @@ static void __build_all_zonelists(void *data)
> > > >  #endif
> > > >  	}
> > > >  
> > > > -	spin_unlock(&lock);
> > > > +	write_sequnlock(&zonelist_update_seq);
> > > >  }
> > > >  
> > > >  static noinline void __init
> > > > 
> > > 
> > > LGTM. The "retry_cpuset" label might deserve a better name now.
> > > 
> > 
> > Good point ...  "restart"?
> > 
> > > Would
> > > 
> > > Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones
> > > with pages managed by the buddy allocator")
> > > 
> > > be correct?
> > > 
> > 
> > Not specifically because the bug is due to a zone being completely removed
> > resulting in a rebuild. This race probably existed ever since memory
> > hotremove could theoritically remove a complete zone. A Cc: Stable would
> > be appropriate as it'll apply with fuzz back to at least 5.4.210 but beyond
> > that, it should be driven by a specific bug report showing that hot-remove
> > of a full zone was possible and triggered the race.
> 
> I do not think so. 6aa303defb74 has changed the zonelist building and
> changed the check from pfn range (populated) to managed (with a memory).

I'm not 100% convinced. The present_pages should have been the spanned range
minus any holes that exist in the zone. If the zone is completely removed,
the span should be zero meaning present and managed are both zero. No? 

The patch *would* have made the situation worse because if a zone was
hot-removed to the point where only reserved pages were present then the
bug would still trigger. On that basis, I'm ok with adding the Fixes: as
it's at least partially true and it covers the range of -stable kernels that
are trivial to backport. Beyond that, it would need greater care and testing.

-- 
Mel Gorman
SUSE Labs