can: isotp: handle wait_event_interruptible() return values

PATCH Superseded Oliver Hartkopp 2 participants 6 messages Jan 4, 2023 – Jan 12, 2023

Subsystems (3)

Stable branch CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2023-01-04 16:51 UTC #0 lore raw

PATCH v2 1/1 can: isotp: handle wait_event_interruptible() return values

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 [email protected] # >= v5.15

Signed-off-by Oliver Hartkopp <[email protected]>

---

V2: fixed checkpatch warnings m(

 net/can/isotp.c | 4 ++++
 1 file changed, 4 insertions(+)

net/can/isotp.c +4 -0

index 0476a506d4a4..fc81d77724a1 100644
@@ -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);

Signature · 1 line

2.30.2

Marc Kleine-Budde

maintainer

2023-01-05 09:32 UTC #1 lore raw

Quoted 6 lines · Oliver Hartkopp

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 [email protected] # >= v5.15
Signed-off-by Oliver Hartkopp <[email protected]>

Can you add a Fixes: tag?

Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2023-01-05 12:59 UTC #2 lore raw

Quoted 9 lines · Marc Kleine-Budde

Quoted 6 lines · Oliver Hartkopp
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 [email protected] # >= v5.15
Signed-off-by Oliver Hartkopp <[email protected]>
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

Marc Kleine-Budde

maintainer

2023-01-06 11:37 UTC #3 lore raw

Quoted 26 lines · Oliver Hartkopp

Quoted 9 lines · Marc Kleine-Budde
Quoted 6 lines · Oliver Hartkopp
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 [email protected] # >= v5.15
Signed-off-by Oliver Hartkopp <[email protected]>
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.

Quoted 6 lines

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

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Oliver Hartkopp

author maintainer

2023-01-06 13:01 UTC #4 lore raw

Quoted 27 lines · Marc Kleine-Budde

Quoted 26 lines · Oliver Hartkopp
Quoted 9 lines · Marc Kleine-Budde
Quoted 6 lines · Oliver Hartkopp
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 [email protected] # >= v5.15
Signed-off-by Oliver Hartkopp <[email protected]>
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.

Quoted 9 lines

Quoted 6 lines
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/[email protected]/T/#u

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/[email protected]/T/#u

Namely:

Fixes 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")

Cc [email protected] # >= 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

Oliver Hartkopp

author maintainer

2023-01-12 19:41 UTC #5 lore raw

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/[email protected]/T/#u

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/[email protected]/T/#u

And the V4 patch.

https://lore.kernel.org/linux-can/[email protected]/T/#u

Quoted 66 lines · Oliver Hartkopp

Quoted 27 lines · Marc Kleine-Budde
Quoted 26 lines · Oliver Hartkopp
Quoted 9 lines · Marc Kleine-Budde
Quoted 6 lines · Oliver Hartkopp
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 [email protected] # >= v5.15
Signed-off-by Oliver Hartkopp <[email protected]>
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.
Quoted 9 lines
Quoted 6 lines
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/[email protected]/T/#u
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/[email protected]/T/#u
Namely:
Fixes 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
Cc [email protected] # >= 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