When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net ---
V2: fixed checkpatch warnings m(
net/can/isotp.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/net/can/isotp.c b/net/can/isotp.c index 0476a506d4a4..fc81d77724a1 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -1150,10 +1150,14 @@ static int isotp_release(struct socket *sock) net = sock_net(sk);
/* wait for complete transmission of current pdu */ wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+ /* force state machines to be idle also when a signal occurred */ + so->tx.state = ISOTP_IDLE; + so->rx.state = ISOTP_IDLE; + spin_lock(&isotp_notifier_lock); while (isotp_busy_notifier == so) { spin_unlock(&isotp_notifier_lock); schedule_timeout_uninterruptible(1); spin_lock(&isotp_notifier_lock);
On 04.01.2023 17:46:05, Oliver Hartkopp wrote:
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net
Can you add a Fixes: tag?
Marc
On 05.01.23 10:32, Marc Kleine-Budde wrote:
On 04.01.2023 17:46:05, Oliver Hartkopp wrote:
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net
Can you add a Fixes: tag?
Yes. Sent out a V3.
In fact I was not sure if it makes sense to apply the patch down to Linux 5.10 as it might increase the possibility to trigger a WARN(1) in the isotp_tx_timer_handler().
The patch is definitely helpful for the latest code including commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames") introduced in Linux 5.18 and its fixes.
I did some testing with very long ISOTP PDUs and killed the waiting isotp_release() with a Crtl-C.
To prevent the WARN(1) we might also stick this patch to
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
What do you think about the WARN(1)?
Best regards, Oliver
On 05.01.2023 13:58:30, Oliver Hartkopp wrote:
On 05.01.23 10:32, Marc Kleine-Budde wrote:
On 04.01.2023 17:46:05, Oliver Hartkopp wrote:
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net
Can you add a Fixes: tag?
Yes. Sent out a V3.
In fact I was not sure if it makes sense to apply the patch down to Linux 5.10 as it might increase the possibility to trigger a WARN(1) in the isotp_tx_timer_handler().
The patch is definitely helpful for the latest code including commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames") introduced in Linux 5.18 and its fixes.
I did some testing with very long ISOTP PDUs and killed the waiting isotp_release() with a Crtl-C.
To prevent the WARN(1) we might also stick this patch to
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
What do you think about the WARN(1)?
If this short patch avoids potential WARN()s it's stable material.
Marc
On 06.01.23 12:37, Marc Kleine-Budde wrote:
On 05.01.2023 13:58:30, Oliver Hartkopp wrote:
On 05.01.23 10:32, Marc Kleine-Budde wrote:
On 04.01.2023 17:46:05, Oliver Hartkopp wrote:
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net
Can you add a Fixes: tag?
Yes. Sent out a V3.
In fact I was not sure if it makes sense to apply the patch down to Linux 5.10 as it might increase the possibility to trigger a WARN(1) in the isotp_tx_timer_handler().
The patch is definitely helpful for the latest code including commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames") introduced in Linux 5.18 and its fixes.
I did some testing with very long ISOTP PDUs and killed the waiting isotp_release() with a Crtl-C.
To prevent the WARN(1) we might also stick this patch to
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
What do you think about the WARN(1)?
If this short patch avoids potential WARN()s it's stable material.
It is the other way around. As written above this patch might increase the possibility to trigger a WARN(1).
With the patch:
https://lore.kernel.org/linux-can/20230104145701.2422-1-socketcan@hartkopp.n...
the WARN(1) is removed and this patch here makes the situation better.
Alternatively I could provide another patch for kernels < v6.0 which set the rx/tx states AND remove the WARN(1).
Back to your original question about the "Fixes:" tag, I would suggest to tag this patch similar to
https://lore.kernel.org/linux-can/20230104145701.2422-1-socketcan@hartkopp.n...
Namely:
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing") Cc: stable@vger.kernel.org # >= v6.0
And later check whether I should remove the WARN(1) in older stable kernels.
Should I send a V4 with the new "Fixes: 866337865f37" tag?
Best regards, Oliver
I sent a V4 (recommended) so that you can pick V3/V4 at your will (see explanation below)
Thanks, Oliver
ps. open patches:
[PATCH can-next] can: isotp: check CAN address family in isotp_bind() https://lore.kernel.org/linux-can/20230104201844.13168-1-socketcan@hartkopp....
The below patch is needed to silence the Syzcaller https://syzkaller.appspot.com/bug?extid=5aed6c3aaba661f5b917
[PATCH] can: isotp: split tx timer into transmission and timeout https://lore.kernel.org/linux-can/20230104145701.2422-1-socketcan@hartkopp.n...
And the V4 patch.
https://lore.kernel.org/linux-can/20230112192347.1944-1-socketcan@hartkopp.n...
On 06.01.23 13:59, Oliver Hartkopp wrote:
On 06.01.23 12:37, Marc Kleine-Budde wrote:
On 05.01.2023 13:58:30, Oliver Hartkopp wrote:
On 05.01.23 10:32, Marc Kleine-Budde wrote:
On 04.01.2023 17:46:05, Oliver Hartkopp wrote:
When wait_event_interruptible() has been interrupted by a signal the tx.state value might not be ISOTP_IDLE. Force the state machines into idle state to inhibit the timer handlers to continue working.
Cc: stable@vger.kernel.org # >= v5.15 Signed-off-by: Oliver Hartkopp socketcan@hartkopp.net
Can you add a Fixes: tag?
Yes. Sent out a V3.
In fact I was not sure if it makes sense to apply the patch down to Linux 5.10 as it might increase the possibility to trigger a WARN(1) in the isotp_tx_timer_handler().
The patch is definitely helpful for the latest code including commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames") introduced in Linux 5.18 and its fixes.
I did some testing with very long ISOTP PDUs and killed the waiting isotp_release() with a Crtl-C.
To prevent the WARN(1) we might also stick this patch to
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
What do you think about the WARN(1)?
If this short patch avoids potential WARN()s it's stable material.
It is the other way around. As written above this patch might increase the possibility to trigger a WARN(1).
With the patch:
https://lore.kernel.org/linux-can/20230104145701.2422-1-socketcan@hartkopp.n...
the WARN(1) is removed and this patch here makes the situation better.
Alternatively I could provide another patch for kernels < v6.0 which set the rx/tx states AND remove the WARN(1).
Back to your original question about the "Fixes:" tag, I would suggest to tag this patch similar to
https://lore.kernel.org/linux-can/20230104145701.2422-1-socketcan@hartkopp.n...
Namely:
Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing") Cc: stable@vger.kernel.org # >= v6.0
And later check whether I should remove the WARN(1) in older stable kernels.
Should I send a V4 with the new "Fixes: 866337865f37" tag?
Best regards, Oliver
linux-stable-mirror@lists.linaro.org
-
Marc Kleine-Budde -
Oliver Hartkopp