Hi Dae,
On 31.03.23 14:06, Dae R. Jeong wrote:
On Fri, Mar 31, 2023 at 12:21:14PM +0200, Oliver Hartkopp wrote:
As discussed with Dae R. Jeong and Hillf Danton here [1] the sendmsg()
function in isotp.c might get into a race condition when restoring the
former tx.state from the old_state. This patch removes the old_state
concept and implements a proper locking for ISOTP_IDLE transitions in
isotp_sendmsg() inspired by a simplification idea from Hillf Danton.
Additionally a new tx.state ISOTP_SHUTDOWN has been introduced to use
the same locking mechanism from isotp_release() which resolves a
potential race between isotp_sendsmg() and isotp_release().
[1] https://lore.kernel.org/linux-can/ZB%2F93xJxq%2FBUqAgG@dragonet/
(..)
+wait_transmission_finish:
/* wait for complete transmission of current pdu */
wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE)
+ goto wait_transmission_finish;
+
/* force state machines to be idle also when a signal occurred */
- so->tx.state = ISOTP_IDLE;
so->rx.state = ISOTP_IDLE;
spin_lock(&isotp_notifier_lock);
while (isotp_busy_notifier == so) {
spin_unlock(&isotp_notifier_lock);
--
2.30.2
All looks good for me. I tried to figure out abnormal thread
interleavings regarding the changes in sendmsg() and release(), but I
couldn't.
I'm still unsure whether a multi-threaded code could send messages and
close the socket at the same time. But I think the above change would
fix a potential race here.
Looking at the code I was wondering if I should also check the return
value of wait_event_interruptible() in isotp_release() and I changed the
code which also removes the goto statement:
/* wait for complete transmission of current pdu */
while (wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE) ==
0 &&
cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE);
/* force state machines to be idle also when a signal occurred */
so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE;
When wait_event_interruptible() does not return '0' we can neither rely
on tx.state to be ISOTP_IDLE nor on anybody else taking care for that.
So I would suggest to continue removing the socket.
Thank you for your hard work!
Thanks for the review and the request to take an additional look at the
code!
Best regards,
Oliver
