[RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[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/

Cc: Dae R. Jeong <threeearcat@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 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

Re: [RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[Dae R. Jeong]

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@gmail.com>
> Cc: Hillf Danton <hdanton@sina.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  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

Re: [RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[Oliver Hartkopp]

Hi Dae,

On 31.03.23 14:06, Dae R. Jeong wrote:
> 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/
>>

(..)

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

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.

> Thank you for your hard work!

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

Best regards,
Oliver

Re: [RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[Dae R. Jeong]

On Fri, Mar 31, 2023 at 9:23 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> Hi Dae,
>
(...)
>
> /* 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?

> /* 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.
>
> > 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

Re: [RFC PATCH] can: isotp: fix race between isotp_sendsmg() and isotp_release()

[Oliver Hartkopp]

On 31.03.23 15:37, Dae R. Jeong wrote:
> On Fri, Mar 31, 2023 at 9:23 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>> Hi Dae,
>>
> (...)
>>
>> /* 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?

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/20230331131935.21465-1-socketcan@hartkopp.net/

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

Best regards,
Oliver

> 
>> /* 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.
>>
>>> 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