Messages in this thread Patch in this message | | | Date | Sun, 26 Mar 2023 13:15:19 +0200 | Subject | Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted | From | Oliver Hartkopp <> |
| |
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;
| |