[PATCH v2] can: isotp: handle wait_event_interruptible() return values

[PATCH v2] can: isotp: handle wait_event_interruptible() return values

[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: 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);
-- 
2.30.2

Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values

[Oliver Hartkopp]

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

Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

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

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values

[Oliver Hartkopp]

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.net/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/20230104145701.2422-1-socketcan@hartkopp.net/T/#u

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

Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values

[Oliver Hartkopp]

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.net/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/20230104145701.2422-1-socketcan@hartkopp.net/T/#u

And the V4 patch.

https://lore.kernel.org/linux-can/20230112192347.1944-1-socketcan@hartkopp.net/T/#u

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.net/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/20230104145701.2422-1-socketcan@hartkopp.net/T/#u
> 
> 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
>