Skip to content

Commit

Permalink
can: isotp: isotp_sendmsg(): fix TX state detection and wait behavior
Browse files Browse the repository at this point in the history
BugLink: https://bugs.launchpad.net/bugs/2046269

[ Upstream commit d9c2ba6 ]

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/[email protected]

Reported-by: Maxime Jayat <[email protected]>
Closes: https://lore.kernel.org/linux-can/[email protected]/
Signed-off-by: Lukas Magel <[email protected]>
Reviewed-by: Oliver Hartkopp <[email protected]>
Fixes: 79e19fa ("can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events")
Link: pylessard/python-udsoncan#178 (comment)
Link: https://lore.kernel.org/all/[email protected]
Signed-off-by: Marc Kleine-Budde <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
Signed-off-by: Kamal Mostafa <[email protected]>
Signed-off-by: Roxana Nicolescu <[email protected]>
  • Loading branch information
lumagi authored and roxanan1996 committed Jan 5, 2024
1 parent da1ebae commit 081252b
Showing 1 changed file with 8 additions and 11 deletions.
19 changes: 8 additions & 11 deletions net/can/isotp.c
Original file line number Diff line number Diff line change
Expand Up @@ -948,21 +948,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;
}

/* PDU size > default => try max_pdu_size */
Expand Down

0 comments on commit 081252b

Please sign in to comment.