Re: [PATCH v3] mm/mempolicy: fix a race between offset_il_node and mpol_rebind_task

[Matthew Wilcox <willy@infradead.org>]

On Mon, Aug 16, 2021 at 05:59:52PM -0700, Andrew Morton wrote:
> On Sun, 15 Aug 2021 14:10:34 +0800 yanghui <yanghui.def@bytedance.com> wrote:
> 
> > Servers happened below panic:
> > Kernel version:5.4.56
> > BUG: unable to handle page fault for address: 0000000000002c48
> > RIP: 0010:__next_zones_zonelist+0x1d/0x40
> > [264003.977696] RAX: 0000000000002c40 RBX: 0000000000100dca RCX: 0000000000000014
> > [264003.977872] Call Trace:
> > [264003.977888]  __alloc_pages_nodemask+0x277/0x310
> > [264003.977908]  alloc_page_interleave+0x13/0x70
> > [264003.977926]  handle_mm_fault+0xf99/0x1390
> > [264003.977951]  __do_page_fault+0x288/0x500
> > [264003.977979]  ? schedule+0x39/0xa0
> > [264003.977994]  do_page_fault+0x30/0x110
> > [264003.978010]  page_fault+0x3e/0x50
> > 
> > The reason of panic is that MAX_NUMNODES is passd in the third parameter
> > in function __alloc_pages_nodemask(preferred_nid). So if to access
> > zonelist->zoneref->zone_idx in __next_zones_zonelist the panic will happen.
> > 
> > In offset_il_node(), first_node() return nid from pol->v.nodes, after
> > this other threads may changed pol->v.nodes before next_node().
> > This race condition will let next_node return MAX_NUMNODES.So put
> > pol->nodes in a local variable.
> > 
> > The race condition is between offset_il_node and cpuset_change_task_nodemask:
> > CPU0:                                     CPU1:
> > alloc_pages_vma()
> >   interleave_nid(pol,)
> >     offset_il_node(pol,)
> >       first_node(pol->v.nodes)            cpuset_change_task_nodemask
> >                       //nodes==0xc          mpol_rebind_task
> >                                               mpol_rebind_policy
> >                                                 mpol_rebind_nodemask(pol,nodes)
> >                       //nodes==0x3
> >       next_node(nid, pol->v.nodes)//return MAX_NUMNODES
> > 
> >
> > ...
> >
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -1965,17 +1965,26 @@ unsigned int mempolicy_slab_node(void)
> >   */
> >  static unsigned offset_il_node(struct mempolicy *pol, unsigned long n)
> >  {
> > -	unsigned nnodes = nodes_weight(pol->nodes);
> > -	unsigned target;
> > +	nodemask_t nodemask = pol->nodes;
> 
> Ouch.  nodemask_t can be large - up to 128 bytes I think.  This looks
> like an expensive thing to be adding to fast paths (alloc_pages_vma()).

Copying a fixed-size 128 bytes to the stack isn't going to be _that_
expensive.

> Plus it consumes a lot of stack.

alloc_pages_vma() tends to be a leaf function, so not that bad.

> > +	unsigned int target, nnodes;
> >  	int i;
> >  	int nid;
> > +	/*
> > +	 * The barrier will stabilize the nodemask in a register or on
> > +	 * the stack so that it will stop changing under the code.
> > +	 *
> > +	 * Between first_node() and next_node(), pol->nodes could be changed
> > +	 * by other threads. So we put pol->nodes in a local stack.
> > +	 */
> > +	barrier();

I think this could be an smp_rmb()?

> > +	nnodes = nodes_weight(nodemask);
> >  	if (!nnodes)
> >  		return numa_node_id();
> >  	target = (unsigned int)n % nnodes;
> > -	nid = first_node(pol->nodes);
> > +	nid = first_node(nodemask);
> >  	for (i = 0; i < target; i++)
> > -		nid = next_node(nid, pol->nodes);
> > +		nid = next_node(nid, nodemask);
> >  	return nid;
> >  }
> 
> The whole idea seems a bit hacky and fragile to be.  We're dealing with
> a potentially stale copy of the nodemask, yes?

Correct.  Also potentially a nodemask in the middle of being changed,
so it may be some unholy amalgam of previous and next.

> Ordinarily this is troublesome because there could be other problems
> caused by working off stale data and a better fix would be to simply
> avoid using stale data!
> 
> But I guess that if the worst case is that once in a billion times,
> interleaving hands out a page which isn't on the intended node then we
> can live with that.
> 
> And if this guess is correct and it is indeed the case that this is the
> worst case, can we please spell all this out in the changelog.

I think that taking a lock here is worse than copying to the stack.
But that seems like the kind of thing that could be measured?

I don't think that working off stale / amalgam data is a bad thing,
we only need consistency.  This is, after all, interleaved allocation.
The user has asked for us, more or less, to choose a node at random to
allocate from.

What ruffles my feathers more is that we call next_node() up to n-2 times,
and on average (n-1)/2 times (where n is the number of permitted nodes).
I can't help but feel that we could do better to randomly distribute
pages between nodes.  Even having a special case for all-bits-set or
n-contiguous-bits-set-and-all-other-bits-clear would go a long way to
speed this up.

I don't know if anyone has a real complaint about how long this takes
to choose a node, though.  I'm loathe to optimise this without data.