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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx/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@xxxxxxxxxxxx/T/#u

And the V4 patch.

https://lore.kernel.org/linux-can/20230112192347.1944-1-socketcan@xxxxxxxxxxxx/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@xxxxxxxxxxxxxxx # >= v5.15
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

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@xxxxxxxxxxxx/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@xxxxxxxxxxxx/T/#u

Namely:

Fixes: 866337865f37 ("can: isotp: fix tx state handling for echo tx processing")
Cc: stable@xxxxxxxxxxxxxxx # >= 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




[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux