On 24.10.23 20:34, Oliver Hartkopp wrote:
> Hello Lukas, hello Greg,
>
> this patch fixed the issue introduced with
>
> 79e19fa79cb5 ("can: isotp: isotp_ops: fix poll() to not report false
> EPOLLOUT events")
>
> for Linux 6.1 and Linux 6.5 which is fine.
>
> Unfortunately the problematic patch has also been applied to 5.15 and
> 5.10 (referencing another upstream commit as it needed a backport).
>
> @Lukas: The 5.x code is much more similar to the latest code, so would
> it probably fix the issue to remove the "wq_has_sleeper(&so->wait)"
> condition?
Yes, the condition is causing the culprit. With the poll patch you mentioned
above, the queue now contains all pollers, even if they're only interested
in reading. So it's not a valid indication of send readiness anymore.
> @Greg: I double checked the changes and fixes from the latest 6.6 kernel
> compared to the 5.10 when isotp.c was introduced in the mainline kernel.
> Would it be ok, to "backport" the latest 6.6 code to the 5.x LTS trees?
> It really is the same isotp code but only some kernel API functions and
> names have been changed.
>
> Best regards,
> Oliver
>
> On 16.10.23 10:40, Greg Kroah-Hartman wrote:
>> 6.1-stable review patch. If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Lukas Magel <lukas.magel@xxxxxxxxxx>
>>
>> [ Upstream commit d9c2ba65e651467de739324d978b04ed8729f483 ]
>>
>> With patch [1], isotp_poll was updated to also queue the poller in the
>> so->wait queue, which is used for send state changes. Since the queue
>> now also contains polling tasks that are not interested in sending, the
>> queue fill state can no longer be used as an indication of send
>> readiness. As a consequence, nonblocking writes can lead to a race and
>> lock-up of the socket if there is a second task polling the socket in
>> parallel.
>>
>> With this patch, isotp_sendmsg does not consult wq_has_sleepers but
>> instead tries to atomically set so->tx.state and waits on so->wait if it
>> is unable to do so. This behavior is in alignment with isotp_poll, which
>> also checks so->tx.state to determine send readiness.
>>
>> V2:
>> - Revert direct exit to goto err_event_drop
>>
>> [1] https://lore.kernel.org/all/20230331125511.372783-1-michal.sojka@xxxxxxx
>>
>> Reported-by: Maxime Jayat <maxime.jayat@xxxxxxxxxxxxxxxxx>
>> Closes: https://lore.kernel.org/linux-can/11328958-453f-447f-9af8-3b5824dfb041@xxxxxxxx/
>> Signed-off-by: Lukas Magel <lukas.magel@xxxxxxxxxx>
>> Reviewed-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
>> Fixes: 79e19fa79cb5 ("can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events")
>> Link: https://github.com/pylessard/python-udsoncan/issues/178#issuecomment-1743786590
>> Link: https://lore.kernel.org/all/20230827092205.7908-1-lukas.magel@xxxxxxxxxx
>> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
>> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
>> ---
>> net/can/isotp.c | 19 ++++++++-----------
>> 1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 8c97f4061ffd7..545889935d39c 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -925,21 +925,18 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>> if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
>> return -EADDRNOTAVAIL;
>>
>> -wait_free_buffer:
>> - /* we do not support multiple buffers - for now */
>> - if (wq_has_sleeper(&so->wait) && (msg->msg_flags & MSG_DONTWAIT))
>> - return -EAGAIN;
>> + while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
>> + /* we do not support multiple buffers - for now */
>> + if (msg->msg_flags & MSG_DONTWAIT)
>> + return -EAGAIN;
>>
>> - /* wait for complete transmission of current pdu */
>> - err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>> - if (err)
>> - goto err_event_drop;
>> -
>> - if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
>> if (so->tx.state == ISOTP_SHUTDOWN)
>> return -EADDRNOTAVAIL;
>>
>> - goto wait_free_buffer;
>> + /* wait for complete transmission of current pdu */
>> + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>> + if (err)
>> + goto err_event_drop;
>> }
>>
>> if (!size || size > MAX_MSG_LENGTH) {
