Re: [PATCH net v4] net/smc: order the CDC receive path against buffer publication

Simon,

> This is a pre-existing issue, but should the unpublish side be fixed in
> the same series, since this patch establishes the publication protocol for
> the ghost buffer?

Real gap, and it is on the list as its own series:

  https://lore.kernel.org/all/[email protected]/

I kept them apart because they are independent revert units with different
Fixes: anchors and stable ranges -- this one is 69cb7dc0218b, where the CDC
path first dereferenced the descriptors; the teardown series is ae2be35cbed2,
which added the ghost buffer. The hunks are disjoint, so either order applies.

There is a coupling worth knowing either way, which I should have said in the
v4 cover: this patch caches sndbuf_desc in a local, so if it lands while the
teardown series has not, that window stops being a NULL dereference and becomes
a read of the freed descriptor. That argues for the teardown series landing
first or alongside, not for merging them. Both orders apply cleanly and give
the same tree; I checked. If you'd rather review them as one series I'll
respin them together.

> Would stopping the tasklet before detaching and freeing the ghost buffer,
> mirroring the publish-last ordering introduced here, close this?

Not on its own. That is what v1 of the teardown patch did, and Dust replied
that it does not fully eliminate the race; he was right. smc_conn_free() calls
smc_ism_unset_conn() only while the link group is still on its device list, and
smc_lgr_terminate_sched() unlinks the group before the worker kills its
connections, so the device can arm the tasklet again after tasklet_kill() has
returned. On an SMC-D loopback rig, with the reorder applied, 78 of 172
connections were still armable when the drain returned and the tasklet was
re-armed 33 times; unregistering first takes that to 0 of 31.

One correction to the review text, since it points at the wrong lock:

> smc_conn_kill() runs from the link group terminate worker with no socket
> lock at all

It does hold it -- __smc_lgr_terminate() takes lock_sock() at smc_core.c:1579,
and f621d6ebeebb did not add that; it only moved the sock_hold() under
conns_lock. The conclusion still stands for a different reason:
smc_cdc_msg_recv() takes only bh_lock_sock() and never checks
sock_owned_by_user(), so it does not defer to the backlog and lock_sock() does
not exclude it.

Thanks for the review.

Bryam