Re: [PATCH net v2 1/2] net/iucv: clear hs_dev before dropping its reference

[Alexandra Winter]

On 29.10.26 05:46, Bryam Vargas via B4 Relay wrote:
> iucv_sock_close() drops the socket's reference on its HiperSockets
> device and only then clears iucv->hs_dev,

I agree on the analysis that this order is wrong.

 while afiucv_hs_rcv() compares
> that field against the ingress device for every socket in the list,
> under iucv_sk_list.lock and not the socket lock. Between the two it
> compares a pointer the socket no longer holds; KCSAN reports the plain
> read against the plain write. 

Could you add an excerpt of the KCSAN report to the commit message?


The order dates from 800c5eb7b5eb, the
> compare from 80230a18c164. No user-visible effect is known: the receive
> path holds the ingress device, so the stale pointer is compared, never
> dereferenced.
> 

I agree, I cannot see how reading a stale pointer can result in a real
problem. So this is not urgent to fix.



> Clear the pointer first, through a local, then drop the reference, and
> mark the lookup's read and the two stores that race with it: the clear
> in close() and the child's store in afiucv_hs_callback_syn() (the child
> is in iucv_sk_list from iucv_sock_alloc() on). The child now takes its
> device and sk_bound_dev_if from the frame the lookup matched, held
> before it is published, instead of reading the listener's fields:
> close() clears them under lock_sock(), which the bh_lock_sock() in
> afiucv_hs_callback_syn() does not exclude, so the callback can pass its
> IUCV_LISTEN check and then read a field close() has cleared. That a SYN
> can be accepted while the listener closes is left to the socket-locking
> rework; this is its KCSAN-visible part. bind()'s hs_dev store runs
> under the list's write lock and stays plain. sk_bound_dev_if, which
> getsockopt() reads without the lock, is marked where close(), bind()
> and the child store it; annotation only.
> 
[...]
> ---
> Nagamani PV's "net/iucv: fix races in afiucv_netdev_event()" [1] marks
> the same store in close(); Alexandra Winter asked him to hold it for the
> locking series and he agreed [2]. This one goes out now, on a new and
> independent KCSAN report, and its close() hunk subsumes his, which marks
> the store but keeps dev_put() first; once this is applied, the
> afiucv_netdev_event() read is what remains for his patch.


Just thinking out loud:
Would it be a simpler alternative to clear iucv->hs_dev  in
iucv_sock_kill after iucv_sock_unlink? then it also would help for the
afiucv_netdev_event() issue (read still needs to be put under lock).

It's hard to discuss this with an AI, while the locking series is still
missing. I would still prefer to fix that first.