can: j1939: unregister_netdevice: waiting for vcan0 to become free.

can: j1939: unregister_netdevice: waiting for vcan0 to become free.

[Tetsuo Handa]

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

Hello.

I found a simplified C reproducer for
https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84 from

  r1 = socket$can_j1939(0x1d, 0x2, 0x7)
  ioctl$ifreq_SIOCGIFINDEX_vcan(r1, 0x8933, &(0x7f0000001440)={'vcan0\x00', <r2=>0x0})
  r3 = socket$can_j1939(0x1d, 0x2, 0x7)
  ioctl$ifreq_SIOCGIFINDEX_vcan(r3, 0x8933, &(0x7f00000000c0)={'vcan0\x00', <r4=>0x0})
  bind$can_j1939(r3, &(0x7f0000000340)={0x1d, r4, 0x0, {0x2, 0x0, 0x6}, 0xfe}, 0x18)
  setsockopt$sock_int(r3, 0x1, 0x6, &(0x7f0000000040)=0x1, 0x4)
  sendmsg$inet(r3, &(0x7f0000000140)={0x0, 0x0, &(0x7f00000003c0)=[{&(0x7f0000000540)="81b641f1f3843704b6", 0x9}], 0x1}, 0x4048081)
  bind$can_j1939(r1, &(0x7f0000000100)={0x1d, r2, 0x0, {0x1, 0xf0, 0x4}, 0xfe}, 0x18)
  setsockopt$sock_int(r1, 0x1, 0x6, &(0x7f0000000040)=0x1, 0x4)
  sendmsg$inet(r3, &(0x7f0000000080)={0x0, 0x0, &(0x7f0000000a80)=[{&(0x7f0000000000)="81b641f1f3843704b6", 0x9}], 0x1}, 0x48005)

lines. Can you find what is wrong?

  [   58.844267] [   T1225] CAN device driver interface
  [   58.865035] [   T1225] vcan: Virtual CAN interface driver
  [   58.924043] [   T1227] can: controller area network core
  [   58.929503] [   T1227] NET: Registered PF_CAN protocol family
  [   58.959118] [   T1228] can: SAE J1939
  [   59.215990] [      C0] vcan0: j1939_tp_rxtimer: 0x0000000042028812: rx timeout, send abort
  [   59.716693] [      C0] vcan0: j1939_tp_rxtimer: 0x0000000041105737: rx timeout, send abort
  [   59.722127] [      C0] vcan0: j1939_tp_rxtimer: 0x0000000042028812: abort rx timeout. Force session deactivation
  [   59.742525] [      C0] vcan0: j1939_xtp_rx_rts_session_active: 0x0000000041105737: connection exists (fe ff). last cmd: 20
  [   59.992638] [      C0] vcan0: j1939_tp_rxtimer: 0x0000000069d7bfc6: rx timeout, send abort
  [   60.497771] [      C0] vcan0: j1939_tp_rxtimer: 0x0000000069d7bfc6: abort rx timeout. Force session deactivation
  [   70.677761] [     T12] unregister_netdevice: waiting for vcan0 to become free. Usage count = 2

[-- Attachment #2: repro.c --]
[-- Type: text/plain, Size: 2728 bytes --]

#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/mman.h>

static void execute_one(void)
{
	int idx = 0;
	const int ONE = 1;
	//  socket$can_j1939 arguments: [
	//    domain: const = 0x1d (8 bytes)
	//    type: const = 0x2 (8 bytes)
	//    proto: const = 0x7 (4 bytes)
	//  ]
	//  returns sock_can_j1939
	const int fd = socket(0x1d, 2, 7);
	//  ioctl$ifreq_SIOCGIFINDEX_vcan arguments: [
	//    fd: sock (resource)
	//    cmd: const = 0x8933 (4 bytes)
	//    arg: ptr[out, ifreq_dev_t[vcan_device_names, ifindex_vcan]] {
	//      ifreq_dev_t[vcan_device_names, ifindex_vcan] {
	//        ifr_ifrn: buffer: {76 63 61 6e 30 00 00 00 00 00 00 00 00 00 00 00}
	//        (length 0x10) elem: ifindex_vcan (resource) pad = 0x0 (20 bytes)
	//      }
	//    }
	//  ]
	memcpy((void*)0x200000001440,
	       "vcan0\000\000\000\000\000\000\000\000\000\000\000", 16);
	if (ioctl(fd, 0x8933, 0x200000001440ul) != -1)
		idx = *(uint32_t*)0x200000001450;
	//  bind$can_j1939 arguments: [
	//    fd: sock_can_j1939 (resource)
	//    addr: ptr[in, sockaddr_can_j1939] {
	//      sockaddr_can_j1939 {
	//        can_family: const = 0x1d (2 bytes)
	//        pad = 0x0 (2 bytes)
	//        can_ifindex: ifindex_vcan (resource)
	//        name: int64 = 0x0 (8 bytes)
	//        pgn: can_j1939_pgn {
	//          pgn_ps: can_j1939_pgn_ps = 0x1 (1 bytes)
	//          pgn_pf: can_j1939_pgn_pf = 0xf0 (1 bytes)
	//          pgn_flags: can_j1939_pgn_flags = 0x4 (1 bytes)
	//          pgn_unused: const = 0x0 (1 bytes)
	//        }
	//        addr: can_j1939_addrs = 0xfe (1 bytes)
	//        pad = 0x0 (3 bytes)
	//      }
	//    }
	//    len: bytesize = 0x18 (8 bytes)
	//  ]
	*(uint16_t*)0x200000000100 = 0x1d;
	*(uint32_t*)0x200000000104 = idx;
	*(uint64_t*)0x200000000108 = 0;
	*(uint8_t*)0x200000000110 = 1;
	*(uint8_t*)0x200000000111 = 0xf0;
	*(uint8_t*)0x200000000112 = 4;
	*(uint8_t*)0x200000000113 = 0;
	*(uint8_t*)0x200000000114 = 0xfe;
	bind(fd, (struct sockaddr *) 0x200000000100ul, 0x18ul);
	setsockopt(fd, SOL_SOCKET, SO_BROADCAST, &ONE, sizeof(ONE));
	send(fd, "\x81\xb6\x41\xf1\xf3\x84\x37\x04\xb6", 9, 0);
	send(fd, "\x81\xb6\x41\xf1\xf3\x84\x37\x04\xb6", 9, 0);
}

int main(int argc, char *argv[])
{
	mmap((void *) 0x200000000000ul, 0x1000000ul,
	     PROT_WRITE|PROT_READ|PROT_EXEC,
	     MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
	if (unshare(CLONE_NEWNET))
		return 1;
	system("ip link add name vcan0 up type vcan");
	system("ip addr add 172.20.20.0/24 dev vcan0");
	execute_one();
	return 0;
}

Re: can: j1939: unregister_netdevice: waiting for vcan0 to become free.

[Tetsuo Handa]

Currently, the (session->last_cmd != 0) path in j1939_xtp_rx_rts_session_active() is
preventing the (session->state == J1939_SESSION_WAITING_ABORT) path in j1939_tp_rxtimer()
 from being called. This results in two j1939_priv refcounts leak (which in turn results in
one net_device refcount leak) due to j1939_session_deactivate_activate_next() being not called.

This problem goes away if I do either

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1689,16 +1692,18 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,

        if (session->last_cmd != 0) {
                /* we received a second rts on the same connection */
-               netdev_alert(priv->ndev, "%s: 0x%p: connection exists (%02x %02x). last cmd: %x\n",
+               netdev_alert(priv->ndev, "%s (modified): 0x%p: connection exists (%02x %02x). last cmd: %x\n",
                             __func__, session, skcb->addr.sa, skcb->addr.da,
                             session->last_cmd);

+               /*
                j1939_session_timers_cancel(session);
                j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
                if (session->transmission)
                        j1939_session_deactivate_activate_next(session);

                return -EBUSY;
+               */
        }

        if (session->skcb.addr.sa != skcb->addr.sa ||

or

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1697,6 +1700,11 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
                j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
                if (session->transmission)
                        j1939_session_deactivate_activate_next(session);
+               else if (session->state == J1939_SESSION_WAITING_ABORT) {
+                       netdev_alert(priv->ndev, "%s (modified): 0x%p: abort rx timeout. Force session deactivation\n",
+                                    __func__, session);
+                       j1939_session_deactivate_activate_next(session);
+               }

                return -EBUSY;
        }

. But what is the correct approach?

Re: can: j1939: unregister_netdevice: waiting for vcan0 to become free.

[Oleksij Rempel]

Hi,

On Tue, Jan 13, 2026 at 12:46:30PM +0900, Tetsuo Handa wrote:
> Currently, the (session->last_cmd != 0) path in j1939_xtp_rx_rts_session_active() is
> preventing the (session->state == J1939_SESSION_WAITING_ABORT) path in j1939_tp_rxtimer()
>  from being called. This results in two j1939_priv refcounts leak (which in turn results in
> one net_device refcount leak) due to j1939_session_deactivate_activate_next() being not called.
> 
> This problem goes away if I do either
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1689,16 +1692,18 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
> 
>         if (session->last_cmd != 0) {
>                 /* we received a second rts on the same connection */
> -               netdev_alert(priv->ndev, "%s: 0x%p: connection exists (%02x %02x). last cmd: %x\n",
> +               netdev_alert(priv->ndev, "%s (modified): 0x%p: connection exists (%02x %02x). last cmd: %x\n",
>                              __func__, session, skcb->addr.sa, skcb->addr.da,
>                              session->last_cmd);
> 
> +               /*
>                 j1939_session_timers_cancel(session);
>                 j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>                 if (session->transmission)
>                         j1939_session_deactivate_activate_next(session);
> 
>                 return -EBUSY;
> +               */
>         }
> 
>         if (session->skcb.addr.sa != skcb->addr.sa ||
> 
> or
> 
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1697,6 +1700,11 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
>                 j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
>                 if (session->transmission)
>                         j1939_session_deactivate_activate_next(session);
> +               else if (session->state == J1939_SESSION_WAITING_ABORT) {

This way looks better for me. May be add a comment like this:
            /* Force deactivation for the receiver.
             * If we rely on the timer starting in j1939_session_cancel, 
             * a second RTS call here will cancel that timer and fail 
             * to restart it because the state is already WAITING_ABORT.
             */

> +                       netdev_alert(priv->ndev, "%s (modified): 0x%p: abort rx timeout. Force session deactivation\n",
> +                                    __func__, session);
> +                       j1939_session_deactivate_activate_next(session);
> +               }
> 
>                 return -EBUSY;
>         }
> 
> . But what is the correct approach?

The second one. Thank you for your work.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] can: j1939: deactivate session upon receiving the second rts

[Tetsuo Handa]

Since j1939_session_deactivate_activate_next() in j1939_tp_rxtimer() is
called only when the timer is enabled, we need to call
j1939_session_deactivate_activate_next() if we cancelled the timer.
Otherwise, refcount for j1939_session leaks, which will later appear as

  unregister_netdevice: waiting for vcan0 to become free. Usage count = 2.

problem.

Reported-by: syzbot <syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 net/can/j1939/transport.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 613a911dda10..8656ab388c83 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1695,8 +1695,16 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 
 		j1939_session_timers_cancel(session);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
-		if (session->transmission)
+		if (session->transmission) {
 			j1939_session_deactivate_activate_next(session);
+		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
+			/* Force deactivation for the receiver.
+			 * If we rely on the timer starting in j1939_session_cancel,
+			 * a second RTS call here will cancel that timer and fail
+			 * to restart it because the state is already WAITING_ABORT.
+			 */
+			j1939_session_deactivate_activate_next(session);
+		}
 
 		return -EBUSY;
 	}
-- 
2.47.3

Re: [PATCH] can: j1939: deactivate session upon receiving the second rts

[Oleksij Rempel]

On Wed, Jan 14, 2026 at 12:28:47AM +0900, Tetsuo Handa wrote:
> Since j1939_session_deactivate_activate_next() in j1939_tp_rxtimer() is
> called only when the timer is enabled, we need to call
> j1939_session_deactivate_activate_next() if we cancelled the timer.
> Otherwise, refcount for j1939_session leaks, which will later appear as
> 
>   unregister_netdevice: waiting for vcan0 to become free. Usage count = 2.
> 
> problem.
> 
> Reported-by: syzbot <syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] can: j1939: deactivate session upon receiving the second rts

[Marc Kleine-Budde]

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

On 14.01.2026 00:28:47, Tetsuo Handa wrote:
> Since j1939_session_deactivate_activate_next() in j1939_tp_rxtimer() is
> called only when the timer is enabled, we need to call
> j1939_session_deactivate_activate_next() if we cancelled the timer.
> Otherwise, refcount for j1939_session leaks, which will later appear as
>
>   unregister_netdevice: waiting for vcan0 to become free. Usage count = 2.
>
> problem.
>
> Reported-by: syzbot <syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Can you provide a Fixes tag? No need to resend. I'll add it while
applying.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH] can: j1939: deactivate session upon receiving the second rts

[Oleksij Rempel]

On Wed, Jan 14, 2026 at 11:22:34AM +0100, Marc Kleine-Budde wrote:
> On 14.01.2026 00:28:47, Tetsuo Handa wrote:
> > Since j1939_session_deactivate_activate_next() in j1939_tp_rxtimer() is
> > called only when the timer is enabled, we need to call
> > j1939_session_deactivate_activate_next() if we cancelled the timer.
> > Otherwise, refcount for j1939_session leaks, which will later appear as
> >
> >   unregister_netdevice: waiting for vcan0 to become free. Usage count = 2.
> >
> > problem.
> >
> > Reported-by: syzbot <syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com>
> > Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> Can you provide a Fixes tag? No need to resend. I'll add it while
> applying.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] can: j1939: deactivate session upon receiving the second rts

[Marc Kleine-Budde]

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

On 14.01.2026 00:28:47, Tetsuo Handa wrote:
> Since j1939_session_deactivate_activate_next() in j1939_tp_rxtimer() is
> called only when the timer is enabled, we need to call
> j1939_session_deactivate_activate_next() if we cancelled the timer.
> Otherwise, refcount for j1939_session leaks, which will later appear as
>
>   unregister_netdevice: waiting for vcan0 to become free. Usage count = 2.
>
> problem.
>
> Reported-by: syzbot <syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Applied to linux-can.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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