[RFC] net/iucv: af_iucv socket locking - accounting, and a staged plan

[RFC] net/iucv: af_iucv socket locking - accounting, and a staged plan

[Bryam Vargas]

Alexandra,

Here is the socket-locking RFC I promised. No patches yet, on purpose: the
accounting turned up one result that changes which fix is right, and I'd
rather agree the direction with you than post a series you have to redirect.

All line numbers are against net-next 0c452fbdf413.


What's there today
------------------

sk->sk_state has 20 assignment sites. One (iucv_sock_alloc:504) runs before the
socket is published on iucv_sk_list at :506, and two set a child socket under
the parent's lock (below). The other 17 are spread across five contexts:

  process, under lock_sock():
    iucv_sock_bind:633,647   iucv_sock_listen:806   iucv_sock_close:411,417,432
    iucv_sock_cleanup_listen:306
  process, under no lock:
    iucv_sock_recvmsg:1338
  iucv tasklet, under bh_lock_sock():
    iucv_callback_txdone:1793   iucv_callback_connrej:1810
  iucv tasklet, under no lock:
    iucv_callback_connack:1708
  NET_RX softirq, under bh_lock_sock():
    afiucv_hs_callback_synack:1941   afiucv_hs_callback_synfin:1961
    afiucv_hs_callback_fin:1983
  qeth TX completion, under no lock:
    afiucv_hs_callback_txnotify:2190,2197
  netdev notifier, under no lock:
    afiucv_netdev_event:2222

Two counts sum it up: sock_owned_by_user() appears zero times in the file, and
there is no socket backlog at all - no .backlog_rcv in iucv_proto, no
sk_add_backlog() anywhere. iucv->backlog_skb_q is a private data queue, not the
socket layer backlog. So the machinery your option A refers to isn't being
mis-used here, it's absent.

Child sockets get their state set under the parent's lock: connreq:1696 and
hs_callback_syn:1917 both set nsk->sk_state while holding bh_lock_sock on the
parent, and iucv_sock_alloc:506 already published the child on iucv_sk_list
before its state was set.


Three gaps, each with a herd7 witness
-------------------------------------

I modelled the locking discipline in LKMM rather than argue it in prose. Six
litmus tests, herd7 7.58. Each RED has a GREEN whose only delta is the lock or
the check, plus a CONTROL so the harness is shown to discriminate:

  RED     bh_lock_sock vs lock_sock, no owner check     Flag data-race
  GREEN   ... same, with sock_owned_by_user() + defer   clean
  RED     recvmsg unlocked, softirq HAS the owner check Flag data-race
  GREEN   ... same, with recvmsg under lock_sock        clean
  RED     a writer holding no lock at all               Flag data-race
  CONTROL two bh_lock_sock writers                      clean

The first pair is your sentence from f558120cd709 ("bh_lock_sock() is not
serializing the tasklet context against process context") as a model verdict.
lock_sock() releases sk_lock.slock before its critical section and leaves only
sk_lock.owned set; bh_lock_sock() holds slock through its critical section. The
two critical sections share no lock, so only an explicit sock_owned_by_user()
check can order them.

The third row is the one the plan turns on, so it's spelled out below. The rest
inline or in a follow-up, whichever you prefer.


The result that changes the plan
--------------------------------

Option A as written in f558120cd709 doesn't reach the receive path.

iucv_sock_recvmsg (1237-1362) takes no socket lock - not lock_sock, not
bh_lock_sock - and it writes sk->sk_state at :1338. Since it never sets
sk_lock.owned, a sock_owned_by_user() check added in the softirq always reads
zero and always takes the "process now" branch. The deferral branch is dead
code against recvmsg. That's the third litmus above; since it's the one the
plan turns on, here it is rather than on request. P1 already has the option-A
fix applied:

  C iucv-recvmsg-vs-hs-callback
  {}

  P0(int *sk_state)
  {
        *sk_state = 5;                  /* recvmsg:1338 -- no socket lock held */
  }

  P1(spinlock_t *slock, int *owned, int *sk_state, int *backlog)
  {
        int r0;

        spin_lock(slock);               /* bh_lock_sock */
        r0 = READ_ONCE(*owned);         /* sock_owned_by_user() */
        if (r0 == 0)
                *sk_state = 2;          /* always taken: recvmsg never sets owned */
        else
                WRITE_ONCE(*backlog, 1);        /* the branch that never runs */
        spin_unlock(slock);
  }

  exists (sk_state=2)

  $ cd tools/memory-model && herd7 -conf linux-kernel.cfg iucv-recvmsg.litmus
  Flag data-race

Give P0 the lock_sock() handshake (set owned under slock, write sk_state, clear
it) and the flag goes away, with nothing else changed.

So the owner check plus re-enqueue fixes the state machine against
close/bind/listen/shutdown, but not against the reader -- and the reader is the
path you asked about.

Putting recvmsg under lock_sock isn't a one-liner either. Two things in the
way, both of which I'd rather you ruled on:

  a) recvmsg:1313 calls iucv_sock_close(), which takes lock_sock:401. Taking
     the lock in recvmsg self-deadlocks there unless the close is split into a
     __iucv_sock_close() that assumes the lock.

     That exact call already cost a CVE: 3589d20a666c ("net/iucv: fix locking
     in .getsockopt", CVE-2026-64004), which you reviewed and tested, fixed a
     NULL deref from getsockopt(SO_MSGSIZE) racing the iucv_sock_close() that
     recvmsg invokes at :1313. Locking getsockopt closed it because
     iucv_sock_close() takes the socket lock itself - which is also exactly why
     recvmsg cannot simply take it too.

  b) skb_recv_datagram() doesn't release the socket lock while it waits, so a
     blocking recv would hold it across the sleep and stall backlog processing
     for the duration. __iucv_sock_wait already has the right shape for this
     (release_sock / schedule_timeout / lock_sock), but it changes recvmsg's
     blocking behaviour.


Staged plan
-----------

Why split rather than send one rework: your caveat in f558120cd709 about return
values and "changes to all users of iucv" applies to the tasklet path, because
those handlers run from iucv_tasklet_fn while holding iucv_table_lock and
iucv_handler is shared with monreader, vmlogrdr, smsgiucv and hvc_iucv. It
doesn't apply to the HiperSockets path: afiucv_hs_rcv is a packet_type handler
in NET_RX softirq that owns the skb, so it can defer to the socket backlog with
no core change and no other driver touched.

Stage 1, HiperSockets receive path. Add iucv_proto.backlog_rcv, hoist
bh_lock_sock from the individual afiucv_hs_callback_* into afiucv_hs_rcv, and
defer via sk_add_backlog() when the socket is owned. Put recvmsg under
lock_sock, with the two items above. Entirely inside af_iucv.c, and it's the
part you named.

Stage 2, iucv tasklet path. This is where option A vs option B actually has to
be answered, and where the core and the four other iucv users are affected.
Option B subsumes it but needs iucv_tasklet_fn to stop holding iucv_table_lock
across handler dispatch, or dispatch moved to the existing iucv_work_fn.

Stage 3, the writers holding no lock. No amount of owner-check discipline at
the other sites reaches these; they need a lock first. iucv_callback_connack is
unambiguous - same tasklet as connrej and shutdown, which already take
bh_lock_sock - so it can just do the same. I have that one ready as a
standalone patch if you want it independently.


Questions
---------

Q1. For stage 1, is putting iucv_sock_recvmsg under lock_sock acceptable to
    you, given (b) changes its blocking behaviour? The alternative is to
    promote message_q.lock to the receive-path lock and move sk_state under it,
    which leaves recvmsg's sleep alone but makes message_q.lock cover something
    it doesn't cover today.

Q2. For stage 2, option A or option B? B is more invasive in net/iucv/iucv.c
    but makes Q1 and stage 3 moot. If you and Hidayath would rather own the
    core change, I'll do stage 1 and hand you the analysis for the rest.

Q3. afiucv_hs_callback_txnotify runs in two contexts, which is why I didn't
    just add a lock to it: qeth_tx_poll (NAPI, softirq) and
    qeth_drain_output_queue (process, teardown) both reach it through
    qeth_notify_skbs. Which one should the locking be written for?


Caveats
-------

I have no IBM Z, so none of this is validated on real HiperSockets. herd7
proves the accesses race under LKMM; it says nothing about memory-safety
impact, which would need a KASAN run on hardware. Treat the model results as an
argument about which lock covers which write, not as a reproduction. I'd rely
on you or Hidayath to validate each revision on real HW before anything lands.

One coordination note: Hidayath's "net/af_iucv: fix use-after-free of listen
sock in iucv_callback_connreq()" is still open, and I'm deliberately not
touching that function. The staged plan resolves connreq as a consequence of
the general direction rather than competing with his patch.

Prior discussion, for anyone picking this up cold:
https://lore.kernel.org/all/20260705-b4-disp-fc79c0dc-v1-1-d2cdcb57afa9@proton.me/
https://lore.kernel.org/all/20260707-b4-disp-783fedbb-v1-1-463b9dbda2ea@proton.me/
https://lore.kernel.org/all/20260706084825.6231-1-hidayath@linux.ibm.com/

Thanks,
Bryam