Re: [PATCH net v2 3/3] net/iucv: send the window update outside message_q.lock

On 22.08.26 13:17, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Redundant concurrent window updates cause spurious socket disconnections due to transient errors.
> - [High] Lockless modification of `sk->sk_state` to `IUCV_DISCONN` in error path corrupts the socket state machine.
> 
> Pre-existing issues:
> - [Critical] Concurrent execution of `iucv_sock_recvmsg` and `iucv_sock_close` can lead to a NULL pointer dereference of `iucv->hs_dev`.
> - [High] Lockless read-modify-write sequence on `sk->sk_shutdown` in `iucv_send_ctrl` permanently loses concurrent state updates.
> --
> 
> --- Patch [3]: [PATCH net v2 3/3] net/iucv: send the window update outside message_q.lock ---
> commit 38dc50c2f4dfcebf699da12476bf1a5fb529484e
> Author: Bryam Vargas <[email protected]>
> 
> net/iucv: send the window update outside message_q.lock
> 
> This patch moves the window update control message dispatch outside the
> message_q.lock in iucv_sock_recvmsg(). It avoids sleeping in atomic context
> since iucv_send_ctrl() performs a GFP_KERNEL allocation. A boolean flag is
> used to defer the send until after the lock is released.
> 
>> diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
>> --- a/net/iucv/af_iucv.c
>> +++ b/net/iucv/af_iucv.c
> [ ... ]
>> @@ -1336,15 +1337,18 @@ static int iucv_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>>  				iucv_process_message_q(sk);
>>  			if (iucv->transport == AF_IUCV_TRANS_HIPER &&
>>  			    atomic_read(&iucv->msg_recv) >=
>> -							iucv->msglimit / 2) {
>> -				err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
>> -				if (err) {
>> -					sk->sk_state = IUCV_DISCONN;
>> -					sk->sk_state_change(sk);
>> -				}
>> -			}
>> +							iucv->msglimit / 2)
>> +				send_win = true;
>>  		}
>>  		spin_unlock_bh(&iucv->message_q.lock);
>> +
>> +		if (send_win) {
>> +			err = iucv_send_ctrl(sk, AF_IUCV_FLAG_WIN);
> 
> [Severity: High]
> Because atomic_read(&iucv->msg_recv) is checked inside the lock, but the
> counter is reset later inside afiucv_hs_send() without holding
> message_q.lock, can multiple concurrent calls to iucv_sock_recvmsg() set
> send_win to true and trigger redundant window updates?
> 

No, because of the previous patch that is mentioned as a precondition.


> If redundant updates are sent, could transient send buffer exhaustion cause
> the socket to unintentionally hit the error path below and disconnect?

I'm not sure I understand the question. But I think this works as designed.