Re: [PATCH net 1/2] net: mptcp: take token lookup references with acquire ordering

[Hang Nan <2122295973@qq.com>]

Thanks.  All four points are correct; respun as v3 with two additional
patches.  Details below.

[High] token 0 accepted by mptcp_token_get_sock()

Agreed, and it is a real defect rather than a consequence of the missing
ordering.  Both validations compare the socket's token with the requested
one, and for a requested token of 0 that comparison is satisfied by
exactly the states a socket must not be accepted in: the value
mptcp_token_destroy() stores, and the zeroed state of a newly allocated
msk.  The ordering change does not help there, because the comparison
itself is vacuous.

What I verified statically before sending v3:

  - mptcp_parse_option() copies the MP_JOIN SYN token with
    get_unaligned_be32() and does not reject 0;
  - subflow_token_join_request() passes the result straight to
    mptcp_pm_get_local_id(), and for a subflow with a different source
    port to mptcp_pm_announced_has_ssk(), which walks
    msk->pm.anno_list under msk->pm.lock;
  - that list is initialised from mptcp_pm_data_init(), i.e. well after
    a socket can be found on a chain.

Patch 3 now rejects token 0 before walking the chain.  I did not manage
to reproduce the resulting crash, so I am not claiming to have observed
it: the description in the patch is a static argument, and the Fixes: tag
points at the commit that introduced the lookup, not at an observed
failure.

[High] mptcp_token_iter_next() re-checks only net_eq()

Agreed.  Patch 4 re-reads the token after the acquisition and drops the
socket unless it is non-zero and still hashes to the slot being walked.
A socket on a chain always hashes to that slot, so the check only rejects
objects whose memory has been reused in the meantime.  I kept the
severity assessment lower than the one above because both consumers
(mptcp_diag, the userspace path manager) are netlink paths that require
privileges in the target network namespace, so the bad state is reached
locally rather than from the wire.

[Medium] the commit message gives the wrong reason

Agreed, and the correction is in patch 1.  The acquire does not pair with
anything in mptcp_token_destroy(): that function does not touch
sk_refcnt.  What it pairs with is the publication in
sock_init_data_uid(), smp_wmb() followed by refcount_set(&sk->sk_refcnt,
1), in the SLAB_TYPESAFE_BY_RCU reuse window, which is the case
refcount.h documents.  The message now says that, and keeps the teardown
comment only as background for what the re-check is looking at.

[Low] Fixes: trailer

Agreed, and corrected in v2 (now v3): 2c5ebd001d4f and 96d890daad05 for
the two mptcp sites, and 1cd62c21572c for the reuseport patch, whose site
also postdates the consolidation.

On testing: the ordering change itself has no runtime observable on
x86-64 (both variants compile to lock cmpxchg) and we have no way to
observe the reordering on the hardware at hand, so patches 1 and 2 carry
an instruction-level argument only (on arm64 the CAS becomes CASA; I can
include the disassembly if useful).  Patches 3 and 4 are plain
functional checks and both objects build; I have not run a KUnit case
for token 0 yet.

pw-bot: cr