Hallo Marc,
can you please upstream this fix?
I just did not monitor the upstream but got back to this patch due to a
discussion on GitHub:
https://github.com/pylessard/python-udsoncan/issues/178#issuecomment-1743786590
Thanks & best regards,
Oliver
On 2023-08-27 14:19, Oliver Hartkopp wrote:
Thanks Lukas!
On 27.08.23 11:22, Lukas Magel wrote:
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")
I would suggest to add this Fixes tag here to give a qualified hint
since when it makes sense to apply this patch from Lukas. It definitely
fixes the introduced isotp_poll() function from Michal.
Btw. before the patch from Michal the poll() syscall was completely
brocken o_O
Many thanks to all of you!
Best regards,
Oliver
---
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 99770ed285..61caf48cf7 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -954,21 +954,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 */
