Hi, I am curious about the error handling logic in isotp_sendmsg() which looks a bit unclear to me. I was looking the `WARNING in isotp_tx_timer_handler` warning [1], which was firstly addressed by a commit [2] but reoccured even after the commit. [1]: https://syzkaller.appspot.com/bug?id=4f492d593461a5e44d76dd9322e179d13191a8ef [2]: c6adf659a8ba can: isotp: check CAN address family in isotp_bind() I thought that the warning is caused by the concurrent execution of two isotp_sendmsg() as described below (I'm not 100% sure though). CPU1 CPU2 isotp_sendmsg() isotp_sendmsg() ----- ----- old_state = so->tx.state; // ISOTP_IDLE cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // success ... so->tx.state = ISTOP_WAIT_FIRST_FC; hrtimer_start(&so->txtimer); cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // failed // if MSG_DONTWAIT is set in msg->msg_flags or // a signal is delivered during wait_event_interruptible() goto err_out; err_out: so->tx.state = old_state; // ISTOP_IDLE isotp_tx_timer_handler() ----- switch (so->tx.state) { default: WARN_ONCE(); } Then, a commit [3] changed the logic of tx timer, and removed the WARN_ONCE() statement. So I thought that the issue is completely handled. [3]: 4f027cba8216 can: isotp: split tx timer into transmission and timeout But even after [3] is applied, I found a warning that seems related occurred [4] (in the kernel commit: 478a351ce0d6). [4]: https://syzkaller.appspot.com/bug?id=11d0e5f6fef53a0ea486bbd07ddd3cba66132150 So I wonder whether the `err_out` logic in isotp_sendmsg() is safe. For me, it looks like isotp_sendmsg() can change so->tx.state to ISTOP_IDLE at any time. It may not be a problem if all other locations are aware of this. Is this an intended behavior? Thank you in advance. Best regards, Dae R. Jeong
