Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi,

On 26.03.23 10:10, Dae R. Jeong wrote:
> Hi,
> 
> I am curious about the error handling logic in isotp_sendmsg() which
> looks a bit unclear to me.
> 
> I was looking the `WARNING in isotp_tx_timer_handler` warning [1],
> which was firstly addressed by a commit [2] but reoccured even after
> the commit.
> [1]: https://syzkaller.appspot.com/bug?id=4f492d593461a5e44d76dd9322e179d13191a8ef
> [2]: c6adf659a8ba can: isotp: check CAN address family in isotp_bind()
> 
> I thought that the warning is caused by the concurrent execution of
> two isotp_sendmsg() as described below (I'm not 100% sure though).
> 
> CPU1                             CPU2
> isotp_sendmsg()                  isotp_sendmsg()
> -----                            -----
> old_state = so->tx.state; // ISOTP_IDLE
> 
>                                   cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // success
> 							     ...
> 							     so->tx.state = ISTOP_WAIT_FIRST_FC;
> 							     hrtimer_start(&so->txtimer);
> 
> cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // failed
> // if MSG_DONTWAIT is set in msg->msg_flags or
> // a signal is delivered during wait_event_interruptible()
> goto err_out;
> err_out:
>      so->tx.state = old_state; // ISTOP_IDLE
> 
>                                   isotp_tx_timer_handler()
> 								 -----
> 								 switch (so->tx.state) {
> 								 default:
> 								     WARN_ONCE();
> 								 }
> 
> Then, a commit [3] changed the logic of tx timer, and removed the
> WARN_ONCE() statement. So I thought that the issue is completely
> handled.
> [3]: 4f027cba8216 can: isotp: split tx timer into transmission and timeout
> 
> But even after [3] is applied, I found a warning that seems related
> occurred [4] (in the kernel commit: 478a351ce0d6).
> [4]: https://syzkaller.appspot.com/bug?id=11d0e5f6fef53a0ea486bbd07ddd3cba66132150
> 
> So I wonder whether the `err_out` logic in isotp_sendmsg() is safe.
> For me, it looks like isotp_sendmsg() can change so->tx.state to
> ISTOP_IDLE at any time. It may not be a problem if all other locations
> are aware of this. Is this an intended behavior?
> 
> Thank you in advance.

Thank you for picking this up!

In fact I was not aware of the possibility of a concurrent execution of 
isotp_sendmsg() and thought cmpxchg() would just make it ...

But looking at other *_sendmsg() implementations a lock_sock() seems to 
be a common pattern to handle concurrent syscalls, see:

git grep -p lock_sock net | grep sendmsg

What do you think about adopting this to isotp_sendmsg()? See patch below.

Best regards,
Oliver

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9bc344851704..0b95c0df7a63 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -912,13 +912,12 @@ static enum hrtimer_restart 
isotp_txfr_timer_handler(struct hrtimer *hrtimer)
  		isotp_send_cframe(so);

  	return HRTIMER_NORESTART;
  }

-static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size)
+static int isotp_sendmsg_locked(struct sock *sk, 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;
@@ -1091,10 +1090,22 @@ static int isotp_sendmsg(struct socket *sock, 
struct msghdr *msg, size_t size)
  		wake_up_interruptible(&so->wait);

  	return err;
  }

+static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, 
size_t size)
+{
+	struct sock *sk = sock->sk;
+	int ret;
+
+	lock_sock(sk);
+	ret = isotp_sendmsg_locked(sk, msg, size);
+	release_sock(sk);
+
+	return ret;
+}
+
  static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, 
size_t size,
  			 int flags)
  {
  	struct sock *sk = sock->sk;
  	struct sk_buff *skb;