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

[Bryam Vargas <hexlabsecurity@proton.me>]

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.

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.

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

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.

Thanks,
Bryam