Re: [linux-next:master] [mm] 47325a5c88: WARNING:at_mm/slub.c:#free_large_kmalloc

[Hugh Dickins]

On Wed, 10 Jul 2024, Usama Arif wrote:
> On 10/07/2024 21:49, Hugh Dickins wrote:
> > It's a long time since I was active hereabouts, but the bot report
> > and your flurry of updates make me think that you should step back,
> > slow down, and look more carefully at the precedents here.
> > 
> > IIRC, the main problem is that parts of the swap_info_struct can
> > still be in use from before while you're wanting to set up new values.
> > Observe how alloc_swap_info() may return a fresh or an old allocation.
> > Observe how enable_swap_info() is called after getting swapon_mutex
> > late in swapon(), once past all possiblities of error.
> > 
> > I expect that your new zeromap needs to be taking the same care as is
> > taken with swap_map and cluster_info: to be safe, follow their example
> > in both swapon() and swapoff().
> > 
> > Hugh
> 
> Thanks, yeah sent too many in quick succession :). Will be more careful next time.
> 
> Both the 2nd and 3rd version are careful to solve the problem of using old allocation
> which you described.
> 
> The 2nd one takes care of it in the same way as swap_map.

It didn't look like that: it set "p->zeromap = zeromap" as soon as
zeromap had been allocated; whereas swap_map is only installed later
via enable_swap_info().

> 
> But I believe its unnecessary to do all that change in the 2nd version, when you can just
> set it to NULL after kvfree, which is a much smaller change and takes care of reusing old
> allocation equally well.

It's possible that there's a good reason why zeromap does not need the
same care as swap_map; and it's possible that things have changed down
the years and swap_map itself doesn't even need all that care.

But you haven't persuaded me: I repeat, step back, slow down,
think carefully about why the existing sequence is as it is
(and please don't respond without doing so).

Hugh