* [PATCH v2] can: isotp: handle wait_event_interruptible() return values
@ 2023-01-04 16:46 Oliver Hartkopp
2023-01-05 9:32 ` Marc Kleine-Budde
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2023-01-04 16:46 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values
2023-01-04 16:46 [PATCH v2] can: isotp: handle wait_event_interruptible() return values Oliver Hartkopp
@ 2023-01-05 9:32 ` Marc Kleine-Budde
2023-01-05 12:58 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-01-05 9:32 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values
2023-01-05 9:32 ` Marc Kleine-Budde
@ 2023-01-05 12:58 ` Oliver Hartkopp
2023-01-06 11:37 ` Marc Kleine-Budde
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2023-01-05 12:58 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values
2023-01-05 12:58 ` Oliver Hartkopp
@ 2023-01-06 11:37 ` Marc Kleine-Budde
2023-01-06 12:59 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2023-01-06 11:37 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values
2023-01-06 11:37 ` Marc Kleine-Budde
@ 2023-01-06 12:59 ` Oliver Hartkopp
2023-01-12 19:31 ` Oliver Hartkopp
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Hartkopp @ 2023-01-06 12:59 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] can: isotp: handle wait_event_interruptible() return values
2023-01-06 12:59 ` Oliver Hartkopp
@ 2023-01-12 19:31 ` Oliver Hartkopp
0 siblings, 0 replies; 6+ messages in thread
From: Oliver Hartkopp @ 2023-01-12 19:31 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, stable
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-12 19:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-04 16:46 [PATCH v2] can: isotp: handle wait_event_interruptible() return values Oliver Hartkopp
2023-01-05 9:32 ` Marc Kleine-Budde
2023-01-05 12:58 ` Oliver Hartkopp
2023-01-06 11:37 ` Marc Kleine-Budde
2023-01-06 12:59 ` Oliver Hartkopp
2023-01-12 19:31 ` Oliver Hartkopp
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).