mirrors/linux
1
0
Fork 1
mirror of https://github.com/torvalds/linux.git synced 2025-11-01 00:58:39 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

can: isotp: fix race between isotp_sendsmg() and isotp_release()

Browse source 
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: Oliver Hartkopp <socketcan@hartkopp.net>
Fixes: 4f027cba82 ("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: Marc Kleine-Budde <mkl@pengutronix.de>
This commit is contained in:
Oliver Hartkopp 2023-03-31 15:19:35 +02:00 committed by Marc Kleine-Budde
parent 79e19fa79c
commit 051737439e
1 changed files with 31 additions and 24 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

55
net/can/isotp.c
View file

@ -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)
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)
{ {
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)
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)
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)
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)
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);