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;
