can: isotp: fix race between isotp_sendsmg() and isotp_release()

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.

Remove the old_state concept and implement proper locking for the
ISOTP_IDLE transitions in isotp_sendmsg(), inspired by a
simplification idea from Hillf Danton.

Introduce a new tx.state ISOTP_SHUTDOWN and 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

v1: https://lore.kernel.org/all/20230331102114.15164-1-socketcan@hartkopp.net
v2: https://lore.kernel.org/all/20230331123600.3550-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_release()
v3: https://lore.kernel.org/all/20230331130654.9886-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_sendmsg() in the wait_tx_done case
v4: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_sendmsg() in ALL cases

Cc: Dae R. Jeong <threeearcat@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Fixes: 4f027cba8216 ("can: isotp: split tx timer into transmission and timeout")
Link: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
Cc: stable@vger.kernel.org
[mkl: rephrase commit message]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 281b7766c54ec8075cf223fd15825512975ebe37..5761d4ab839dd9776f072ba9ce78ac969defc97d 100644 (file)
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -119,7 +119,8 @@ enum {
        ISOTP_WAIT_FIRST_FC,
        ISOTP_WAIT_FC,
        ISOTP_WAIT_DATA,
-       ISOTP_SENDING
+       ISOTP_SENDING,
+       ISOTP_SHUTDOWN,
 };
 
 struct tpcon {
@@ -880,8 +881,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
                                             txtimer);
        struct sock *sk = &so->sk;
 
-       /* don't handle timeouts in IDLE state */
-       if (so->tx.state == ISOTP_IDLE)
+       /* don't handle timeouts in IDLE or SHUTDOWN state */
+       if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
                return HRTIMER_NORESTART;
 
        /* we did not get any flow control or echo frame in time */
@@ -918,7 +919,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 {
        struct sock *sk = sock->sk;
        struct isotp_sock *so = isotp_sk(sk);
-       u32 old_state = so->tx.state;
        struct sk_buff *skb;
        struct net_device *dev;
        struct canfd_frame *cf;
@@ -928,23 +928,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
        int off;
        int err;
 
-       if (!so->bound)
+       if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
                return -EADDRNOTAVAIL;
 
+wait_free_buffer:
        /* we do not support multiple buffers - for now */
-       if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE ||
-           wq_has_sleeper(&so->wait)) {
-               if (msg->msg_flags & MSG_DONTWAIT) {
-                       err = -EAGAIN;
-                       goto err_out;
-               }
+       if (wq_has_sleeper(&so->wait) && (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_out;
+       /* wait for complete transmission of current pdu */
+       err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+       if (err)
+               goto err_event_drop;
 
-               so->tx.state = ISOTP_SENDING;
+       if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
+               if (so->tx.state == ISOTP_SHUTDOWN)
+                       return -EADDRNOTAVAIL;
+
+               goto wait_free_buffer;
        }
 
        if (!size || size > MAX_MSG_LENGTH) {
@@ -1074,7 +1075,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
        if (wait_tx_done) {
                /* wait for complete transmission of current pdu */
-               wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+               err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+               if (err)
+                       goto err_event_drop;
 
                if (sk->sk_err)
                        return -sk->sk_err;
@@ -1082,13 +1085,15 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
        return size;
 
+err_event_drop:
+       /* got signal: force tx state machine to be idle */
+       so->tx.state = ISOTP_IDLE;
+       hrtimer_cancel(&so->txfrtimer);
+       hrtimer_cancel(&so->txtimer);
 err_out_drop:
        /* drop this PDU and unlock a potential wait queue */
-       old_state = ISOTP_IDLE;
-err_out:
-       so->tx.state = old_state;
-       if (so->tx.state == ISOTP_IDLE)
-               wake_up_interruptible(&so->wait);
+       so->tx.state = ISOTP_IDLE;
+       wake_up_interruptible(&so->wait);
 
        return err;
 }
@@ -1150,10 +1155,12 @@ static int isotp_release(struct socket *sock)
        net = sock_net(sk);
 
        /* wait for complete transmission of current pdu */
-       wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+       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_IDLE;
+       so->tx.state = ISOTP_SHUTDOWN;
        so->rx.state = ISOTP_IDLE;
 
        spin_lock(&isotp_notifier_lock);