The LTP test pty03 is causing a crash in slcan:
BUG: kernel NULL pointer dereference, address: 0000000000000008
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 0 PID: 348 Comm: kworker/0:3 Not tainted 6.0.8-1-default #1 openSUSE Tumbleweed 9d20364b934f5aab0a9bdf84e8f45cfdfae39dab
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
Workqueue: 0x0 (events)
RIP: 0010:process_one_work (/home/rich/kernel/linux/kernel/workqueue.c:706 /home/rich/kernel/linux/kernel/workqueue.c:2185)
Code: 49 89 ff 41 56 41 55 41 54 55 53 48 89 f3 48 83 ec 10 48 8b 06 48 8b 6f 48 49 89 c4 45 30 e4 a8 04 b8 00 00 00 00 4c 0f 44 e0 <49> 8b 44 24 08 44 8b a8 00 01 00 00 41 83 e5 20 f6 45 10 04 75 0e
RSP: 0018:ffffaf7b40f47e98 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff9d644e1b8b48 RCX: ffff9d649e439968
RDX: 00000000ffff8455 RSI: ffff9d644e1b8b48 RDI: ffff9d64764aa6c0
RBP: ffff9d649e4335c0 R08: 0000000000000c00 R09: ffff9d64764aa734
R10: 0000000000000007 R11: 0000000000000001 R12: 0000000000000000
R13: ffff9d649e4335e8 R14: ffff9d64490da780 R15: ffff9d64764aa6c0
FS: 0000000000000000(0000) GS:ffff9d649e400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000008 CR3: 0000000036424000 CR4: 00000000000006f0
Call Trace:
<TASK>
worker_thread (/home/rich/kernel/linux/kernel/workqueue.c:2436)
kthread (/home/rich/kernel/linux/kernel/kthread.c:376)
ret_from_fork (/home/rich/kernel/linux/arch/x86/entry/entry_64.S:312)
Apparently, the slcan's tx_work is freed while being scheduled. While
slcan_netdev_close() (netdev side) calls flush_work(&sl->tx_work),
slcan_close() (tty side) does not. So when the netdev is never set UP,
but the tty is stuffed with bytes and forced to wakeup write, the work
is scheduled, but never flushed.
So add an additional flush_work() to slcan_close() to be sure the work
is flushed under all circumstances.
The Fixes commit below moved flush_work() from slcan_close() to
slcan_netdev_close(). What was the rationale behind it? Maybe we can
drop the one in slcan_netdev_close()?
I see the same pattern in can327. So it perhaps needs the very same fix.
Fixes: cfcb4465e992 ("can: slcan: remove legacy infrastructure")
Link: https://bugzilla.suse.com/show_bug.cgi?id=1205597
Reported-by: Richard Palethorpe <[email protected]>
Tested-by: Petr Vorel <[email protected]>
Cc: Dario Binacchi <[email protected]>
Cc: Wolfgang Grandegger <[email protected]>
Cc: Marc Kleine-Budde <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Max Staudt <[email protected]>
Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
---
drivers/net/can/slcan/slcan-core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index fbb34139daa1..f4db77007c13 100644
--- a/drivers/net/can/slcan/slcan-core.c
+++ b/drivers/net/can/slcan/slcan-core.c
@@ -864,12 +864,14 @@ static void slcan_close(struct tty_struct *tty)
{
struct slcan *sl = (struct slcan *)tty->disc_data;
- /* unregister_netdev() calls .ndo_stop() so we don't have to.
- * Our .ndo_stop() also flushes the TTY write wakeup handler,
- * so we can safely set sl->tty = NULL after this.
- */
unregister_candev(sl->dev);
+ /*
+ * The netdev needn't be UP (so .ndo_stop() is not called). Hence make
+ * sure this is not running before freeing it up.
+ */
+ flush_work(&sl->tx_work);
+
/* Mark channel as dead */
spin_lock_bh(&sl->lock);
tty->disc_data = NULL;
--
2.38.1
Hi all,
Cc LTP mailing list.
Jiri, thanks a lot for quick fix!
Kind regards,
Petr
> The LTP test pty03 is causing a crash in slcan:
> BUG: kernel NULL pointer dereference, address: 0000000000000008
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 348 Comm: kworker/0:3 Not tainted 6.0.8-1-default #1 openSUSE Tumbleweed 9d20364b934f5aab0a9bdf84e8f45cfdfae39dab
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
> Workqueue: 0x0 (events)
> RIP: 0010:process_one_work (/home/rich/kernel/linux/kernel/workqueue.c:706 /home/rich/kernel/linux/kernel/workqueue.c:2185)
> Code: 49 89 ff 41 56 41 55 41 54 55 53 48 89 f3 48 83 ec 10 48 8b 06 48 8b 6f 48 49 89 c4 45 30 e4 a8 04 b8 00 00 00 00 4c 0f 44 e0 <49> 8b 44 24 08 44 8b a8 00 01 00 00 41 83 e5 20 f6 45 10 04 75 0e
> RSP: 0018:ffffaf7b40f47e98 EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff9d644e1b8b48 RCX: ffff9d649e439968
> RDX: 00000000ffff8455 RSI: ffff9d644e1b8b48 RDI: ffff9d64764aa6c0
> RBP: ffff9d649e4335c0 R08: 0000000000000c00 R09: ffff9d64764aa734
> R10: 0000000000000007 R11: 0000000000000001 R12: 0000000000000000
> R13: ffff9d649e4335e8 R14: ffff9d64490da780 R15: ffff9d64764aa6c0
> FS: 0000000000000000(0000) GS:ffff9d649e400000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000008 CR3: 0000000036424000 CR4: 00000000000006f0
> Call Trace:
> <TASK>
> worker_thread (/home/rich/kernel/linux/kernel/workqueue.c:2436)
> kthread (/home/rich/kernel/linux/kernel/kthread.c:376)
> ret_from_fork (/home/rich/kernel/linux/arch/x86/entry/entry_64.S:312)
> Apparently, the slcan's tx_work is freed while being scheduled. While
> slcan_netdev_close() (netdev side) calls flush_work(&sl->tx_work),
> slcan_close() (tty side) does not. So when the netdev is never set UP,
> but the tty is stuffed with bytes and forced to wakeup write, the work
> is scheduled, but never flushed.
> So add an additional flush_work() to slcan_close() to be sure the work
> is flushed under all circumstances.
> The Fixes commit below moved flush_work() from slcan_close() to
> slcan_netdev_close(). What was the rationale behind it? Maybe we can
> drop the one in slcan_netdev_close()?
> I see the same pattern in can327. So it perhaps needs the very same fix.
> Fixes: cfcb4465e992 ("can: slcan: remove legacy infrastructure")
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1205597
> Reported-by: Richard Palethorpe <[email protected]>
> Tested-by: Petr Vorel <[email protected]>
> Cc: Dario Binacchi <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Max Staudt <[email protected]>
> Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
> ---
> drivers/net/can/slcan/slcan-core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index fbb34139daa1..f4db77007c13 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -864,12 +864,14 @@ static void slcan_close(struct tty_struct *tty)
> {
> struct slcan *sl = (struct slcan *)tty->disc_data;
> - /* unregister_netdev() calls .ndo_stop() so we don't have to.
> - * Our .ndo_stop() also flushes the TTY write wakeup handler,
> - * so we can safely set sl->tty = NULL after this.
> - */
> unregister_candev(sl->dev);
> + /*
> + * The netdev needn't be UP (so .ndo_stop() is not called). Hence make
> + * sure this is not running before freeing it up.
> + */
> + flush_work(&sl->tx_work);
> +
> /* Mark channel as dead */
> spin_lock_bh(&sl->lock);
> tty->disc_data = NULL;
(CC: [email protected] because Petr did so.)
Hi Jiry,
Thanks for finding this!
Your patch looks correct to me, so please have a
Reviewed-by: Max Staudt <[email protected]>
for both this patch to slcan, as well as an 1:1 patch to can327.
Some history:
This is actually my code from can327, which was backported to slcan as
part of Dario's larger modernisation effort.
The rationale for moving it was to flush the UART TX buffer in case of
ndo_close(), in order to bring the device into a more predictable state
between ndo_close() and ndo_open(). I guess that's actually
counterproductive - whatever is in the TX buffer at that time should
likely be fully sent. For example, can327 sends one last byte to abort
any running chatty monitoring mode from the adapter. So your patch also
fixes this as well ;)
Of course, this resulted in calling flush_worker() in both ndo_stop()
and ldisc_close(), so I wanted to avoid code duplication, and relied on
ndo_stop(). Oops.
Thanks,
Max
On 02.12.2022 03:52:42, Max Staudt wrote:
> (CC: [email protected] because Petr did so.)
>
> Hi Jiry,
>
> Thanks for finding this!
>
>
> Your patch looks correct to me, so please have a
>
> Reviewed-by: Max Staudt <[email protected]>
>
> for both this patch to slcan, as well as an 1:1 patch to can327.
Max, can you create a patch for the can327 driver?
regards,
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 |
On 01.12.2022 08:34:26, Jiri Slaby (SUSE) wrote:
> The LTP test pty03 is causing a crash in slcan:
> BUG: kernel NULL pointer dereference, address: 0000000000000008
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 348 Comm: kworker/0:3 Not tainted 6.0.8-1-default #1 openSUSE Tumbleweed 9d20364b934f5aab0a9bdf84e8f45cfdfae39dab
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
> Workqueue: 0x0 (events)
> RIP: 0010:process_one_work (/home/rich/kernel/linux/kernel/workqueue.c:706 /home/rich/kernel/linux/kernel/workqueue.c:2185)
> Code: 49 89 ff 41 56 41 55 41 54 55 53 48 89 f3 48 83 ec 10 48 8b 06 48 8b 6f 48 49 89 c4 45 30 e4 a8 04 b8 00 00 00 00 4c 0f 44 e0 <49> 8b 44 24 08 44 8b a8 00 01 00 00 41 83 e5 20 f6 45 10 04 75 0e
> RSP: 0018:ffffaf7b40f47e98 EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff9d644e1b8b48 RCX: ffff9d649e439968
> RDX: 00000000ffff8455 RSI: ffff9d644e1b8b48 RDI: ffff9d64764aa6c0
> RBP: ffff9d649e4335c0 R08: 0000000000000c00 R09: ffff9d64764aa734
> R10: 0000000000000007 R11: 0000000000000001 R12: 0000000000000000
> R13: ffff9d649e4335e8 R14: ffff9d64490da780 R15: ffff9d64764aa6c0
> FS: 0000000000000000(0000) GS:ffff9d649e400000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000008 CR3: 0000000036424000 CR4: 00000000000006f0
> Call Trace:
> <TASK>
> worker_thread (/home/rich/kernel/linux/kernel/workqueue.c:2436)
> kthread (/home/rich/kernel/linux/kernel/kthread.c:376)
> ret_from_fork (/home/rich/kernel/linux/arch/x86/entry/entry_64.S:312)
>
> Apparently, the slcan's tx_work is freed while being scheduled. While
> slcan_netdev_close() (netdev side) calls flush_work(&sl->tx_work),
> slcan_close() (tty side) does not. So when the netdev is never set UP,
> but the tty is stuffed with bytes and forced to wakeup write, the work
> is scheduled, but never flushed.
>
> So add an additional flush_work() to slcan_close() to be sure the work
> is flushed under all circumstances.
>
> The Fixes commit below moved flush_work() from slcan_close() to
> slcan_netdev_close(). What was the rationale behind it? Maybe we can
> drop the one in slcan_netdev_close()?
>
> I see the same pattern in can327. So it perhaps needs the very same fix.
>
> Fixes: cfcb4465e992 ("can: slcan: remove legacy infrastructure")
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1205597
> Reported-by: Richard Palethorpe <[email protected]>
> Tested-by: Petr Vorel <[email protected]>
> Cc: Dario Binacchi <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Max Staudt <[email protected]>
> Signed-off-by: Jiri Slaby (SUSE) <[email protected]>
Added to linux-can,
Thanks,
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 |
This patch fixes the crash, but is IMHO incomplete: The flush_work() in
.ndo_stop() should also be removed, since its existence implies
unexpected behaviour.
In other words, my moving it there in can327 was a double mistake, and
slcan just happened to copy my mistake over.
I'm preparing a patch for can327, and it will remove the flush from
.ndo_stop(). What shall we do about slcan?
Max