can: isotp: split tx timer into transmission and timeout

The timer for the transmission of isotp PDUs formerly had two functions:
1. send two consecutive frames with a given time gap
2. monitor the timeouts for flow control frames and the echo frames

This led to larger txstate checks and potentially to a problem discovered
by syzbot which enabled the panic_on_warn feature while testing.

The former 'txtimer' function is split into 'txfrtimer' and 'txtimer'
to handle the two above functionalities with separate timer callbacks.

The two simplified timers now run in one-shot mode and make the state
transitions (especially with isotp_rcv_echo) better understandable.

Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
Reported-by: syzbot+5aed6c3aaba661f5b917@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org # >= v6.0
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://lore.kernel.org/all/20230104145701.2422-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/net/can/isotp.c b/net/can/isotp.c
index dae421f6c901c15594aafd2a282a41853a8655ff..fc81d77724a1310e9caebfbfb125f1f61de00e13 100644 (file)
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -140,7 +140,7 @@ struct isotp_sock {
        canid_t rxid;
        ktime_t tx_gap;
        ktime_t lastrxcf_tstamp;
-       struct hrtimer rxtimer, txtimer;
+       struct hrtimer rxtimer, txtimer, txfrtimer;
        struct can_isotp_options opt;
        struct can_isotp_fc_options rxfc, txfc;
        struct can_isotp_ll_options ll;
@@ -871,7 +871,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
        }
 
        /* start timer to send next consecutive frame with correct delay */
-       hrtimer_start(&so->txtimer, so->tx_gap, HRTIMER_MODE_REL_SOFT);
+       hrtimer_start(&so->txfrtimer, so->tx_gap, HRTIMER_MODE_REL_SOFT);
 }
 
 static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
@@ -879,49 +879,39 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
        struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
                                             txtimer);
        struct sock *sk = &so->sk;
-       enum hrtimer_restart restart = HRTIMER_NORESTART;
 
-       switch (so->tx.state) {
-       case ISOTP_SENDING:
+       /* don't handle timeouts in IDLE state */
+       if (so->tx.state == ISOTP_IDLE)
+               return HRTIMER_NORESTART;
 
-               /* cfecho should be consumed by isotp_rcv_echo() here */
-               if (!so->cfecho) {
-                       /* start timeout for unlikely lost echo skb */
-                       hrtimer_set_expires(&so->txtimer,
-                                           ktime_add(ktime_get(),
-                                                     ktime_set(ISOTP_ECHO_TIMEOUT, 0)));
-                       restart = HRTIMER_RESTART;
+       /* we did not get any flow control or echo frame in time */
 
-                       /* push out the next consecutive frame */
-                       isotp_send_cframe(so);
-                       break;
-               }
-
-               /* cfecho has not been cleared in isotp_rcv_echo() */
-               pr_notice_once("can-isotp: cfecho %08X timeout\n", so->cfecho);
-               fallthrough;
+       /* report 'communication error on send' */
+       sk->sk_err = ECOMM;
+       if (!sock_flag(sk, SOCK_DEAD))
+               sk_error_report(sk);
 
-       case ISOTP_WAIT_FC:
-       case ISOTP_WAIT_FIRST_FC:
+       /* reset tx state */
+       so->tx.state = ISOTP_IDLE;
+       wake_up_interruptible(&so->wait);
 
-               /* we did not get any flow control frame in time */
+       return HRTIMER_NORESTART;
+}
 
-               /* report 'communication error on send' */
-               sk->sk_err = ECOMM;
-               if (!sock_flag(sk, SOCK_DEAD))
-                       sk_error_report(sk);
+static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)
+{
+       struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
+                                            txfrtimer);
 
-               /* reset tx state */
-               so->tx.state = ISOTP_IDLE;
-               wake_up_interruptible(&so->wait);
-               break;
+       /* start echo timeout handling and cover below protocol error */
+       hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
+                     HRTIMER_MODE_REL_SOFT);
 
-       default:
-               WARN_ONCE(1, "can-isotp: tx timer state %08X cfecho %08X\n",
-                         so->tx.state, so->cfecho);
-       }
+       /* cfecho should be consumed by isotp_rcv_echo() here */
+       if (so->tx.state == ISOTP_SENDING && !so->cfecho)
+               isotp_send_cframe(so);
 
-       return restart;
+       return HRTIMER_NORESTART;
 }
 
 static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
@@ -1198,6 +1188,7 @@ static int isotp_release(struct socket *sock)
                }
        }
 
+       hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);
        hrtimer_cancel(&so->rxtimer);
 
@@ -1601,6 +1592,8 @@ static int isotp_init(struct sock *sk)
        so->rxtimer.function = isotp_rx_timer_handler;
        hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
        so->txtimer.function = isotp_tx_timer_handler;
+       hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+       so->txfrtimer.function = isotp_txfr_timer_handler;
 
        init_waitqueue_head(&so->wait);
        spin_lock_init(&so->rx_lock);