Re: [PATCH v3 10/12] mempolicy: alloc_pages_mpol() for NUMA policy without vma

[Hugh Dickins]

On Mon, 23 Oct 2023, Johannes Weiner wrote:
> On Mon, Oct 23, 2023 at 08:10:32PM +0200, domenico cerasuolo wrote:
> > Il giorno lun 23 ott 2023 alle ore 19:53 Andrew Morton
> > <akpm@xxxxxxxxxxxxxxxxxxxx> ha scritto:
> > >
> > > On Mon, 23 Oct 2023 18:53:26 +0200 domenico cerasuolo <mimmocerasuolo@xxxxxxxxx> wrote:
> > >
> > > > > Rebased to mm.git's current mm-stable, to resolve with removal of
> > > > > vma_policy() from include/linux/mempolicy.h, and temporary omission
> > > > > of Nhat's ZSWAP mods from mm/swap_state.c: no other changes.
> > > >
> > > > Hi Hugh,
> > > >
> > > > not sure if it's the rebase, but I don't see an update to
> > > > __read_swap_cache_async invocation in zswap.c at line 1078. Shouldn't we pass a
> > > > mempolicy there too?
> > >
> > > No change needed.  zswap_writeback_entry() was passing a NULL for arg
> > > `vma' and it's now passing a NULL for arg `mpol'.

Andrew's answer was indeed my thinking, and why none of us got a build error.

> > 
> > Problem is that alloc_pages_mpol is dereferencing mpol, when I test the zswap
> > writeback at 397148729f21edcf700ecb2a01749dbce955d09e it crashes, not sure if
> > I'm missing something.
> 
> I don't think you are. The NULL vma used to go to get_vma_policy(),
> which fell back to
> 
> 	pol = get_task_policy(current);
> 
> Now the NULL pol gets passed to alloc_pages_mpol() directly, which
> dereferences it. Oops.

Yes, I failed to think it through that far.

> 
> I think Hugh's patch needs zswap to pass get_task_policy(current)
> instead of NULL.

That sounds the likely fix, thank you Domenico, Andrew, Johannes.

I'll check it out and send a fix patch later today.

I don't know who runs that zswap_writeback_entry() code, but I presume
that task's mempolicy is unlikely to be relevant to the swap cache page
in question: but a whole lot better than oopsing, and will reproduce
the previous behaviour (and the assumption at this writeback point would
be that the page is unlikely to be reused after writeback anyway, so its
node unimportant).

Hugh