can: isotp: fix race between isotp_sendsmg() and isotp_release()
Subsystems (2)
Oliver Hartkopp
maintainerRFC PATCH v3 1/1 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. 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/
---
v2: take care of signal interrupts for wait_event_interruptible() in
isotp_release()
v3: take care of signal interrupts for wait_event_interruptible() in
isotp_sendmsg() in the wait_tx_done case
net/can/isotp.c | 55 ++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-)
net/can/isotp.c +31 -24
@@ -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,29 @@ 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) { +/* got signal: force tx state machine to be idle */ + so->tx.state =ISOTP_IDLE ; +hrtimer_cancel (&so->txfrtimer); +hrtimer_cancel (&so->txtimer); + goto err_out_drop; + }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,
@@ -1148,14 +1153,16 @@ static int isotp_release(struct socket *sock) so =isotp_sk (sk); 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);while (isotp_busy_notifier == so) {spin_unlock (&isotp_notifier_lock);
Signature · 1 line
2.30.2