[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

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

[Alexandra Winter]

On 25.07.26 00:29, Bryam Vargas wrote:
> 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.
> 

Bryam,
thank you very much for this structured approach at this pile of work.

> 
> 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.
> 

Yes, I was aware of that :-/

> 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.


Yes, as discussed in your initial patch, I am aware that iucv_sock_recvmsg is
currently not protected.


> 
> 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.


I understand. Yes, that would be the way to go.


> 
>      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.


I understand. I'm not sure I can judge all the implications, but your proposal makes
sense to me.

I wonder, if there is more required than taking and releasing the socket lock.
e.g. I think recvmsg needs to check (sk->sk_state == IUCV_CONNECTED) before
iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN).
But let's take it one by one.


> 
> 
> 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.


Sounds good to me

> 
> 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.
> 

That would be great, we are also working on several small fixes. Let's get them out of the way.

> 
> 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.

No, I don't like that. message_q is a concept for iucv not HS - I see message_q.lock
is already used in both paths, but I don't want to stretch it further.
I prefer your proposal above and would like to test it with KASAN etc.

> 
> 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.


I would prefer option A: keep the tasklet and use bh_lock_sock(). You seem to
think that is doable?
Less invasive is attractive.


> 
> 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?
> 

This is a tough one. Maybe we need to add a context parameter to qeth_notify_skbs() ?



> 
> 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.


Yes, we will validate any revision.


> 
> 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
> 
> 

Thanks again for working on this. You identified several workitems, are there any
where you would prefer me or Hidayath to work on?
Otherwise we will continue with issues that are not on this list and work on
running and improving our testcases.

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

[Bryam Vargas]

Alexandra,

Four things in the RFC were wrong. Two of them change what I asked you to
decide, so they go first.

The data races. I called them that on the strength of herd7, and that was
wrong. sk_state is volatile unsigned char skc_state, and LKMM raises its
data-race flag only when one side of a conflict is a plain access, so a
volatile access never qualifies. My litmus encoded the writes as plain, which
manufactured the flag - and the flag was the only thing separating the bug arms
from the fixed ones. That set did not show what I said it showed. I rebuilt it
three times and every rebuild described code that does not exist; the last one
paired a writer that only fires on the classic transport with a waker that only
fires on HiperSockets, which no single socket can be. There is no replacement
litmus. The unsynchronized writes are still unsynchronized and that part reads
straight from the source.

"The owner check plus re-enqueue fixes the state machine against
close/bind/listen/shutdown". It does not fix close. iucv_sock_close() sets
IUCV_CLOSING at :417 and then, when !err and skbs_in_xmit is non-zero at :420,
sleeps in iucv_sock_wait() for IUCV_CLOSED. That macro does release_sock()
before schedule_timeout() and lock_sock() after, so sk_lock.owned is clear for
the whole sleep, up to IUCV_DISCONN_TIMEOUT. A sock_owned_by_user() check in a
softirq writer reads false across that window, and release_sock() would drain a
deferred write into it on the way out. Option A orders the tasklet against a
process context holding the lock and does nothing for one sleeping inside it.
Tell me if I have misread iucv_sock_wait().

"Stage 2 affects the core and the four other iucv users". It doesn't have to.
Below.

The hardware caveat was too broad. iucv_packet_type is registered with
dev_add_pack() on every af_iucv init and carries no .dev, so an ETH_P_AF_IUCV
frame on any netdev reaches afiucv_hs_rcv, and the dispatch from there branches
only on trans_hdr->flags. The softirq sk_state writers are drivable in a plain
s390x guest. What needs your hardware is the classic transport and the real
qeth TX-completion contexts.

> I would prefer option A: keep the tasklet and use bh_lock_sock(). You seem to
> think that is doable?
> Less invasive is attractive.

Yes, with one ordering constraint and one shortcut.

The constraint is stage 1, which you already signed off on. Option A can't
order the tasklet against the reader until recvmsg holds the socket lock:
recvmsg sets no sk_lock.owned today, so the deferral branch has nothing to test
and never fires against it. Stage 1 is a prerequisite for stage 2, not a
parallel track.

The shortcut removes the cost you flagged in f558120cd709, "this may require
adding return values to the tasklet functions and thus changes to all users of
iucv". struct proto has .release_cb, and net/smc already uses it
(smc_release_cb). af_iucv can set iucv_proto.release_cb and keep a per-socket
mask of pending state changes: af_iucv.c plus one field in af_iucv.h, no return
values, nothing in net/iucv/iucv.c, and monreader, vmlogrdr, smsgiucv and
hvc_iucv untouched.

I prototyped it on an s390x kernel: a writer that finds sock_owned_by_user()
true sets a bit instead of touching sk_state, and release_sock() applies it --
sk_state 1 -> 5, defer flags cleared. Synthetic __init probe, not the real
handler path; it shows the deferral vehicle works, nothing more. Log on request.

release_cb runs under sk_lock.slock with BH disabled, so whatever gets deferred
there must not sleep. Writing sk_state and waking the state-change waiters is
fine.

> No, I don't like that. message_q is a concept for iucv not HS - I see message_q.lock
> is already used in both paths, but I don't want to stretch it further.
> I prefer your proposal above and would like to test it with KASAN etc.

Agreed, and I'd drop that alternative. Worth adding that message_q.lock already
reaches further than it should on that path: iucv_sock_recvmsg() calls
iucv_send_ctrl() with it held, and iucv_send_ctrl() allocates through
sock_alloc_send_skb(), which uses sk->sk_allocation - GFP_KERNEL for these
sockets. On a default-msglimit HiperSockets socket the msglimit/2 gate fires at
64 receives, so it's not a corner case. I have a DEBUG_ATOMIC_SLEEP splat for
it from an s390x guest, from the same __init probe as above with msg_recv set
rather than received: the state is staged, the sleeping allocation under
spin_lock_bh is not.

> I wonder, if there is more required than taking and releasing the socket lock.
> e.g. I think recvmsg needs to check (sk->sk_state == IUCV_CONNECTED) before
> iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN).
> But let's take it one by one.

The check belongs there, and I'd put it in with the lock rather than before it.
Unlocked it narrows the window without closing it: iucv_sock_close() clears
hs_dev under lock_sock while recvmsg holds nothing, so the gap between the test
and iucv_send_ctrl() stays open. It becomes sound once recvmsg holds the socket
lock, which is where stage 1 puts it.

iucv_send_ctrl() has one more defect at that call site, independent of the
locking: five callers, and this is the only one not gated on the transport, so a
classic z/VM socket sends a HiperSockets control frame it has no device for. It
sizes the skb via LL_RESERVED_SPACE(iucv->hs_dev), afiucv_hs_send() returns
-ENODEV on a null skb->dev, and recvmsg turns that into IUCV_DISCONN. It takes
SO_MSGLIMIT set to 1 to get there: msg_recv is only incremented on the
HiperSockets path, so on a classic socket it stays zero and the msglimit/2 test
passes only when msglimit is 1. On a guest booted without relocate_lowcore the
read through the null hs_dev lands in the mapped lowcore instead of faulting,
which is why I saw a spurious disconnect and not an oops; with lowcore
relocation that read would fault.

> This is a tough one. Maybe we need to add a context parameter to qeth_notify_skbs() ?

Yes, and one of the two obvious candidates doesn't work.

qeth_notify_skbs() has two calling functions. qeth_iqd_tx_complete() reaches it
three times and has a single caller, qeth_tx_poll(), so it is NAPI only.
qeth_tx_complete_pending_bufs() reaches it once and has two callers:
qeth_drain_output_queue() with drain=true, and qeth_tx_poll() with drain=false.
The drain path is the only non-NAPI reach, so drain is already an exact
discriminator at that call site and it's in scope there. budget is not:
netpoll calls napi->poll(napi, 0) from atomic context, so budget == 0 happens on
both sides. qeth_tx_complete_buf() on the next line passes budget to
napi_consume_skb(), correct there and wrong here.

No single primitive covers both contexts, which is why the parameter is
unavoidable: bh_lock_sock() is a plain spin_lock() on sk_lock.slock with no BH
disable, so it's unsafe from the drain path, and lock_sock() sleeps, so it's
unsafe from NAPI. sk_txnotify is an af_iucv/qeth private pointer, so the
signature change touches af_iucv.h, af_iucv.c and qeth_core_main.c and none of
the four other iucv_handler users.

I also read your reply on Nagamani PV's afiucv_netdev_event() patch, and I agree
with the call. The notifier runs in process context, so lock_sock() is available
there; the two-line fix closes the traversal use-after-free and leaves
sk->sk_state = IUCV_DISCONN and sk->sk_state_change() in the loop body with no
socket lock. I had that site filed as stage 3, on the assumption it needed a
lock chosen for it. Process context makes it the same shape as stage 1 -
lock_sock, owned_by_user, backlog - so it folds into the combined fix.

> That would be great, we are also working on several small fixes. Let's get them out of the way.

The connack patch is written: bh_lock_sock() in iucv_callback_connack(), the one
site whose context is unambiguous, since connrej and shutdown run in the same
tasklet and already take it. Fixes: eac3731bd04c - the function has held no lock
since 2007. checkpatch --strict is quiet and it builds for s390x. Which tree do
you want it against?

> Thanks again for working on this. You identified several workitems, are there any
> where you would prefer me or Hidayath to work on?
> Otherwise we will continue with issues that are not on this list and work on
> running and improving our testcases.

You already answered the only thing I would have asked for: you will validate
any revision. The analysis I can do here; z/VM and real HiperSockets I can't,
and QEMU is not z/VM, so machine_is_vm() is false and the classic transport is
not reachable at all. If you would rather own the stage 2 change yourselves,
take it.

Thanks,
Bryam

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

[Alexandra Winter]

On 09.08.26 03:49, Bryam Vargas wrote:
>> That would be great, we are also working on several small fixes. Let's get them out of the way.
> The connack patch is written: bh_lock_sock() in iucv_callback_connack(), the one
> site whose context is unambiguous, since connrej and shutdown run in the same
> tasklet and already take it. Fixes: eac3731bd04c - the function has held no lock
> since 2007. checkpatch --strict is quiet and it builds for s390x. Which tree do
> you want it against?

net, please.