Re: [PATCH mptcp-next 2/2] mptcp: pm: fix memory leak from alloc-during-teardown race

[Shardul Bankar <shardul.b@mpiricsoftware.com>]

Hi Matt,

Thanks for the review, and for flagging Sashiko's note.

On Mon, 2026-06-22 at 19:37 +0200, Matthieu Baerts wrote:
> Hi Shardul,
> 
> Thank you for the v4!
> 
> (Note: please make sure to keep the version in the prefix -- b4 does
> that for you automatically ;) -- to help with the patch tracking)
> 

Yes I'll be more careful, thanks for pointing it out.

> On 20/06/2026 10:44, Shardul Bankar wrote:
> > mptcp_pm_destroy() empties msk->pm.anno_list and
> > msk->pm.userspace_pm_local_addr_list under msk->pm.lock during
> > socket
> > teardown, dropping the lock between the two.
> > 
> > A concurrent userspace PM genl ANNOUNCE on the same msk holds a
> > sock
> > reference via mptcp_token_get_sock() and, in
> > mptcp_pm_nl_announce_doit(), calls
> > mptcp_userspace_pm_append_new_local_addr() and
> > mptcp_pm_announced_alloc(). Both take msk->pm.lock briefly to add
> > to
> > their respective lists. Because the genl handler holds a sock
> > reference,
> > mptcp_pm_destroy() may run on the same msk via mptcp_disconnect(),
> > which
> > invokes mptcp_destroy_common() without dropping the sock refcount,
> > before the handler completes.
> 
> Sashiko seems to say [1] that the mptcp_disconnect() path is not
> covered
> (when 'shutdown()' then 'connect()' is called from the userspace),
> but
> it looks like that's what you are covering it. Just to be sure: is it
> a
> false positive? (I didn't check the different code path to see if
> that's
> possible)
> 
> [1]
> https://sashiko.dev/#/patchset/20260620084424.3072634-1-shardul.b%40mpiricsoftware.com?part=2
> 
> Cheers,
> Matt


It's a false positive for the leak, and your reading is right.

The factual part of the note is true: mptcp_pm_destroy() sets
MPTCP_PM_DESTROYING, but mptcp_disconnect() then calls
mptcp_pm_data_reset(), whose memset(&pm->reset, ...) clears the bit
(status lives in the reset struct_group). So the bit deliberately does
not survive a disconnect(). It just doesn't cause a leak, because the
two teardown paths are covered by two different mechanisms:

- Final teardown: __mptcp_destroy_sock() sets pm.status = 0 and then
calls sk_prot->destroy() -> mptcp_destroy_common() ->
mptcp_pm_destroy(), which sets DESTROYING under pm.lock right around
the list free. There is no pm_data_reset() afterwards, so the bit
persists. A concurrent ANNOUNCE
(mptcp_userspace_pm_append_new_local_addr() runs under pm.lock only,
before announce_doit() takes lock_sock()) either runs before the free
and gets reclaimed, or sees DESTROYING and aborts. This is the race the
fence exists for.

- disconnect()/reuse: the userspace list free in mptcp_pm_destroy() is
unconditional (patch 1/2) and runs on every teardown. An entry appended
after the disconnect-time reset lands on the reused, still-live socket
and is reclaimed by the unconditional free at that socket's next
teardown. So it is never orphaned. The fence bit being reset here is
harmless by design.

Thanks,
Shardul