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

RFC PATCH Superseded Oliver Hartkopp 2 participants 5 messages Mar 31, 2023

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2023-03-31 10:24 UTC #0 lore raw

RFC PATCH 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/

Cc Dae R. Jeong <[email protected]>

Cc Hillf Danton <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/isotp.c | 49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

net/can/isotp.c +26 ~~-23~~

index 9bc344851704..2ba043f5a9cb 100644
@@ -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);

Signature · 1 line

2.30.2

Dae R. Jeong

2023-03-31 12:06 UTC #1 lore raw

Quoted 156 lines · Oliver Hartkopp

[1] https://lore.kernel.org/linux-can/ZB%2F93xJxq%2FBUqAgG@dragonet/

Cc Dae R. Jeong <[email protected]>

Cc Hillf Danton <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/isotp.c | 49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

net/can/isotp.c +26 ~~-23~~

index 9bc344851704..2ba043f5a9cb 100644
@@ -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);

Signature · 1 line

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

Oliver Hartkopp

author maintainer

2023-03-31 12:23 UTC #2 lore raw

Hi Dae,

Quoted 12 lines · Dae R. Jeong

Quoted 11 lines · Oliver Hartkopp
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/

(..)

Quoted 12 lines

net/can/isotp.c +4 -1

+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;

Quoted 10 lines

Quoted 6 lines
spin_lock(&isotp_notifier_lock); while (isotp_busy_notifier == so) {
spin_unlock(&isotp_notifier_lock);
Signature · 1 line
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.

I'm still unsure whether a multi-threaded code could send messages and close the socket at the same time. But I think the above change would fix a potential race here.

Looking at the code I was wondering if I should also check the return value of wait_event_interruptible() in isotp_release() and I changed the code which also removes the goto statement:

/* wait for complete transmission of current pdu */
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_SHUTDOWN;
so->rx.state = ISOTP_IDLE;

When wait_event_interruptible() does not return '0' we can neither rely on tx.state to be ISOTP_IDLE nor on anybody else taking care for that.

So I would suggest to continue removing the socket.

Quoted 1 line

Thank you for your hard work!

Thanks for the review and the request to take an additional look at the code!

Best regards,
Oliver

Dae R. Jeong

2023-03-31 13:37 UTC #3 lore raw

Quoted 3 lines · Oliver Hartkopp

Hi Dae,

(...)

Quoted 5 lines

/* wait for complete transmission of current pdu */
while (wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE) == 0 &&
        cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE);

I'm not sure, but your intention in this condition is probably while (wait_event_interruptible() != 0 || cmpxchg() != ISOTP_IDLE)? So, release() can get out of the loop only if wait_event_interruptible() returns 0 and cmpxchg() successes?

Quoted 9 lines

/* force state machines to be idle also when a signal occurred */ so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE;
When wait_event_interruptible() does not return '0' we can neither rely on tx.state to be ISOTP_IDLE nor on anybody else taking care for that.
So I would suggest to continue removing the socket.

Quoted 7 lines

Quoted 1 line
Thank you for your hard work!
Thanks for the review and the request to take an additional look at the code!
Best regards,
Oliver

Best regards,
Dae R. Jeong

Oliver Hartkopp

author maintainer

2023-03-31 15:26 UTC #4 lore raw

Quoted 11 lines · Dae R. Jeong

Quoted 3 lines · Oliver Hartkopp
Hi Dae,
(...)
Quoted 5 lines
/* wait for complete transmission of current pdu */
while (wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE) == 0 &&
         cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SHUTDOWN) != ISOTP_IDLE);

Quoted 4 lines

I'm not sure, but your intention in this condition is probably while (wait_event_interruptible() != 0 || cmpxchg() != ISOTP_IDLE)? So, release() can get out of the loop only if wait_event_interruptible() returns 0 and cmpxchg() successes?

No it is the other way around.

When wait_event_interruptible() returns 0 the state has been properly set to ISOTP_IDLE (the good case).

And THEN we try to grab the tx.state to be ISOTP_SHUTDOWN.

The while() statement is left when either the tx.state is ISOTP_SHUTDOWN (after it was ISOTP_IDLE) 'OR' when a signal occurred which terminated wait_event_interruptible().

In the latter case we do not know the value from tx.state.

Therefore I set the tx.state to ISOTP_IDLE/ISOTP_SHUTDOWN in the V4 patch after checking each wait_event_interruptible() return value:

https://lore.kernel.org/linux-can/[email protected]/

I think this is the best way to handle the signal interrupt case for wait_event_interruptible() ?!?

Best regards,
Oliver

Quoted 20 lines

Quoted 10 lines
/* force state machines to be idle also when a signal occurred */ so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE;
When wait_event_interruptible() does not return '0' we can neither rely on tx.state to be ISOTP_IDLE nor on anybody else taking care for that.
So I would suggest to continue removing the socket.
Quoted 7 lines
Quoted 1 line
Thank you for your hard work!
Thanks for the review and the request to take an additional look at the code!
Best regards,
Oliver
Best regards,
Dae R. Jeong