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

[Hidayath Khan]

On 29/08/26 12:04 am, Bryam Vargas wrote:
> Hidayath,
>
> > Let me know how you'd like to sequence it.
> Don't sequence them behind me. Send the accept_q_lock patch now, and take
> iucv_callback_connreq() back. I asked for it on the 21st assuming the rework
> would move, and it won't for a while: I'm in the middle of a larger project and
> have only a few hours a week for this at the moment. Holding two patches and a
> leak fix for that is a bad trade for you.
I appreciate the advice, but I am going to hold onto these patches for 
the time being.
The consensus is that we land your core socket locking rework first before
tackling the remaining gaps. We want to avoid sending partial fixes that 
merely
narrow race windows, as that will make the underlying bugs harder to 
reproduce and
add confusion.

It also gives me time to properly rework the patch:
[PATCH net] net/af_iucv: fix use-after-free of listen sock in 
iucv_callback_connreq()
and confirm all reproducers.

> One thing worth having before the walk fix goes out. The lock on the walk is
> necessary and it isn't sufficient. Both producers link the child and set its
> state afterwards: iucv_accept_enqueue() at af_iucv.c:1916, then
> nsk->sk_state = IUCV_CONNECTED at :1917, and the same order at :1693 and :1696.
> So the state store sits outside accept_q_lock, and a walker holding that lock
> still reads sk_state at :1372 with nothing ordering the two. iucv_accept_poll()
> tests exactly that field to decide EPOLLIN, so a poll landing between :1916 and
> :1917 sees a child that is on the queue and not yet CONNECTED. Moving the state
> store ahead of the enqueue in both producers closes it. The lock alone narrows
> the window.
Agreed. The planned accept_q series incorporates your feedback
to ensure the state ordering and walk serialization close the race 
completely.

- Patch 1: Takes accept_q_lock inside iucv_accept_poll().
- Patch 2: Explicitly reorders the producers (iucv_callback_connreq and
  afiucv_hs_callback_syn) to set nsk->sk_state = IUCV_CONNECTED before
  calling iucv_accept_enqueue(), ensuring a locked walker never reads
  an un-updated state.
- Patch 3: Extends proper accept_q_lock serialization to the sleeping
  walk inside iucv_accept_dequeue().

> > while iucv_accept_enqueue() appends from the IUCV tasklet and the
> > HiperSockets softirq under bh_lock_sock(parent)
> Small correction that doesn't change your conclusion: the append's own lock is
> spin_lock_irqsave(&par->accept_q_lock) at :516-518, not bh_lock_sock. The walk
> takes neither, so the finding stands either way -- it matters only for where
> the fix goes.
Thanks for pointing out the distinction on accept_q_lock vs bh_lock_sock 
- noted.
> You're right about iucv_accept_dequeue() too. Its walk at :542 is equally
> unlocked and can't be wrapped the same way, because the loop body sleeps in
> lock_sock() at :544.
>
> All of that is source plus a model, not a run. I have no Z.
I will rebase these patches on top of Stage 1 once it lands.

Thanks,
Hidayath
> Thanks,
> Bryam