Commit 05173743 authored by Oliver Hartkopp's avatar Oliver Hartkopp Committed by Marc Kleine-Budde

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.

Remove the old_state concept and implement proper locking for the
ISOTP_IDLE transitions in isotp_sendmsg(), inspired by a
simplification idea from Hillf Danton.

Introduce a new tx.state ISOTP_SHUTDOWN and 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

v1: https://lore.kernel.org/all/20230331102114.15164-1-socketcan@hartkopp.net
v2: https://lore.kernel.org/all/20230331123600.3550-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_release()
v3: https://lore.kernel.org/all/20230331130654.9886-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_sendmsg() in the wait_tx_done case
v4: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
    take care of signal interrupts for wait_event_interruptible() in
    isotp_sendmsg() in ALL cases

Cc: Dae R. Jeong <threeearcat@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Signed-off-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
Fixes: 4f027cba ("can: isotp: split tx timer into transmission and timeout")
Link: https://lore.kernel.org/all/20230331131935.21465-1-socketcan@hartkopp.net
Cc: stable@vger.kernel.org
[mkl: rephrase commit message]
Signed-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent 79e19fa7
...@@ -119,7 +119,8 @@ enum { ...@@ -119,7 +119,8 @@ enum {
ISOTP_WAIT_FIRST_FC, ISOTP_WAIT_FIRST_FC,
ISOTP_WAIT_FC, ISOTP_WAIT_FC,
ISOTP_WAIT_DATA, ISOTP_WAIT_DATA,
ISOTP_SENDING ISOTP_SENDING,
ISOTP_SHUTDOWN,
}; };
struct tpcon { struct tpcon {
...@@ -880,8 +881,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer) ...@@ -880,8 +881,8 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
txtimer); txtimer);
struct sock *sk = &so->sk; struct sock *sk = &so->sk;
/* don't handle timeouts in IDLE state */ /* don't handle timeouts in IDLE or SHUTDOWN state */
if (so->tx.state == ISOTP_IDLE) if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
return HRTIMER_NORESTART; return HRTIMER_NORESTART;
/* we did not get any flow control or echo frame in time */ /* we did not get any flow control or echo frame in time */
...@@ -918,7 +919,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -918,7 +919,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{ {
struct sock *sk = sock->sk; struct sock *sk = sock->sk;
struct isotp_sock *so = isotp_sk(sk); struct isotp_sock *so = isotp_sk(sk);
u32 old_state = so->tx.state;
struct sk_buff *skb; struct sk_buff *skb;
struct net_device *dev; struct net_device *dev;
struct canfd_frame *cf; struct canfd_frame *cf;
...@@ -928,23 +928,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -928,23 +928,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
int off; int off;
int err; int err;
if (!so->bound) if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
return -EADDRNOTAVAIL; return -EADDRNOTAVAIL;
wait_free_buffer:
/* we do not support multiple buffers - for now */ /* we do not support multiple buffers - for now */
if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE || if (wq_has_sleeper(&so->wait) && (msg->msg_flags & MSG_DONTWAIT))
wq_has_sleeper(&so->wait)) { return -EAGAIN;
if (msg->msg_flags & MSG_DONTWAIT) {
err = -EAGAIN;
goto err_out;
}
/* wait for complete transmission of current pdu */ /* wait for complete transmission of current pdu */
err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
if (err) if (err)
goto err_out; goto err_event_drop;
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) { if (!size || size > MAX_MSG_LENGTH) {
...@@ -1074,7 +1075,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -1074,7 +1075,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
if (wait_tx_done) { if (wait_tx_done) {
/* wait for complete transmission of current pdu */ /* 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)
goto err_event_drop;
if (sk->sk_err) if (sk->sk_err)
return -sk->sk_err; return -sk->sk_err;
...@@ -1082,13 +1085,15 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) ...@@ -1082,13 +1085,15 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
return size; return size;
err_event_drop:
/* got signal: force tx state machine to be idle */
so->tx.state = ISOTP_IDLE;
hrtimer_cancel(&so->txfrtimer);
hrtimer_cancel(&so->txtimer);
err_out_drop: err_out_drop:
/* drop this PDU and unlock a potential wait queue */ /* drop this PDU and unlock a potential wait queue */
old_state = ISOTP_IDLE; so->tx.state = ISOTP_IDLE;
err_out: wake_up_interruptible(&so->wait);
so->tx.state = old_state;
if (so->tx.state == ISOTP_IDLE)
wake_up_interruptible(&so->wait);
return err; return err;
} }
...@@ -1150,10 +1155,12 @@ static int isotp_release(struct socket *sock) ...@@ -1150,10 +1155,12 @@ static int isotp_release(struct socket *sock)
net = sock_net(sk); net = sock_net(sk);
/* wait for complete transmission of current pdu */ /* wait for complete transmission of current pdu */
wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); 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 */ /* force state machines to be idle also when a signal occurred */
so->tx.state = ISOTP_IDLE; so->tx.state = ISOTP_SHUTDOWN;
so->rx.state = ISOTP_IDLE; so->rx.state = ISOTP_IDLE;
spin_lock(&isotp_notifier_lock); spin_lock(&isotp_notifier_lock);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment