Re: [PATCH net 2/2] mptcp: close race between scheduler and state change

[Paolo Abeni <pabeni@redhat.com>]

On 9/16/26 22:45, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] Incomplete fix: the identical `mss_now == 0` divide-by-zero that
>    the patch guards in `mptcp_push_release()` (net/mptcp/protocol.c:1667)…
> 
> --
> 
> mptcp: close race between scheduler and state change
> 
> The patch guards the tcp_push() call in mptcp_push_release() with a check
> for info->mss_now being non-zero, so that a subflow which failed
> transmission (leaving mss_now at 0) is not used for a divide operation.
> 
> Closes: https://lore.kernel.org/20260525194828.1137119-1-shardul.b@mpiricsoftware.com
> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 0098e28309318..709610db3ce9d 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1664,7 +1664,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>>   
>>   static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
>>   {
>> -	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
>> +	if (info->mss_now)
>> +		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle,
>> +			 info->size_goal);
>>   	release_sock(ssk);
>>   }
> 
> [Severity: High]
> Should the same guard also be added to the second tcp_push() call site, in
> __mptcp_subflow_push_pending()?
> 
> That function starts with a zeroed info:
> 
> net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
> 	struct mptcp_sendmsg_info info = {
> 		.data_lock_held = true,
> 	};
> 	...
> 	int copied = 0;
> 
> and info.mss_now is only ever assigned inside mptcp_sendmsg_frag(), after
> the early return the commit message refers to:
> 
> 	if (unlikely(!__tcp_can_send(ssk)))
> 		return -EAGAIN;
> 
> 	/* compute send limit */
> 	...
> 	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);
> 
> In the non-first iteration, the return value is accumulated into copied
> without filtering negatives:
> 
> net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
> 		if (READ_ONCE(subflow->scheduled)) {
> 			mptcp_subflow_set_scheduled(subflow, false);
> 			ret = __subflow_push_pending(sk, ssk, &info);
> 			if (ret <= 0)
> 				keep_pushing = false;
> 			copied += ret;
> 		}
> 
> __subflow_push_pending() passes mptcp_sendmsg_frag()'s -EAGAIN through
> verbatim when the first frag fails, so copied becomes -EAGAIN, which is
> non-zero, while info.mss_now is still 0:
This is a pre-existing issue but it could deserve to be fixed in the 
same series.

/P