Re: [PATCH net] net/smc: drain the rx tasklet before detaching the ghost sndbuf

[Bryam Vargas <hexlabsecurity@proton.me>]

Dust,

> I think Sashiko's review makes sense. Your change doesn't seem to fully
> eliminate the race here. I'm wondering if this should be solved with
> RCU rather than just moving smcd_buf_detach() later?

You're right, it doesn't. I built both shapes and measured them on an SMC-D
loopback rig. "path" is connections reaching either teardown site with the link
group already unlinked, "armable" how many of those still had both gates in
smcd_handle_irq() open when the drain returned, "re-armed" the device arming
the tasklet again afterwards:

  form                                   path  armable  re-armed
  upstream                                169       73        29
  v1 (drain, then detach)                 172       78        33
  unregister first, then drain             31        0         0
  v1 + RCU                                 24        9         3

Two things about that table before you read anything into it. The last two arms
ran much shorter than the first two, so the ratios are the comparable part, not
the absolute path counts. And the third arm also forced tasklet_kill() in the
!soft path; that was inert here because smc_lgr_terminate_work() passes
soft=true, but it wasn't only the unregister.

The reorder leaves the window open because 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 it before the worker kills the
connections. It never sets conn->killed either, so both gates stay open across
tasklet_kill().

RCU does not close it. smcd_buf_detach() frees the descriptor and clears the
field, and RCU defers only the free -- the re-armed tasklet still runs and
still finds conn->sndbuf_desc NULL. RCU on the descriptor would still be a
reasonable thing to want for the free itself; it just isn't what shuts this
window, so I didn't fold it in. Say the word if you want it anyway.

v2 unregisters unconditionally before the drain, and clears conn->sndbuf_desc
before freeing it so a reader that samples the pointer can't get a freed one.
It carries a second patch for a dereference the same teardown reaches:
smc_close_stream_wait() calls smc_tx_prepared_sends() from inside
sk_wait_event(), which evaluates its condition once with the socket lock
released. SIOCOUTQ reads the field by hand, so that one is bounded too.

Thanks for catching it.

Bryam