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/ > > Cc: Dae R. Jeong <threeearcat@xxxxxxxxx> > Cc: Hillf Danton <hdanton@xxxxxxxx> > Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> > --- > net/can/isotp.c | 49 ++++++++++++++++++++++++++----------------------- > 1 file changed, 26 insertions(+), 23 deletions(-) > > diff --git a/net/can/isotp.c b/net/can/isotp.c > index 9bc344851704..2ba043f5a9cb 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -117,11 +117,12 @@ MODULE_ALIAS("can-proto-6"); > enum { > ISOTP_IDLE = 0, > ISOTP_WAIT_FIRST_FC, > ISOTP_WAIT_FC, > ISOTP_WAIT_DATA, > - ISOTP_SENDING > + ISOTP_SENDING, > + ISOTP_SHUTDOWN, > }; > > struct tpcon { > unsigned int idx; > unsigned int len; > @@ -878,12 +879,12 @@ 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; > > - /* 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 */ > > /* report 'communication error on send' */ > @@ -916,37 +917,37 @@ static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer) > > 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; > int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0; > int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0; > s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT; > 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) > + return err; > > - 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) { > err = -EINVAL; > goto err_out_drop; > @@ -1072,25 +1073,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) > goto err_out_drop; > } > > 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) > + return err; > > if (sk->sk_err) > return -sk->sk_err; > } > > return size; > > 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; > } > > static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size, > @@ -1147,15 +1147,18 @@ static int isotp_release(struct socket *sock) > return 0; > > so = isotp_sk(sk); > net = sock_net(sk); > > +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. Thank you for your hard work! Best regards, Dae R. Jeong
