From: Ziyang Xuan <hidden> Date: 2021-09-06 09:42:58
The conclusion "j1939_session_deactivate() should be called with a
session ref-count of at least 2" is incorrect. In some concurrent
scenarios, j1939_session_deactivate can be called with the session
ref-count less than 2. But there is not any problem because it
will check the session active state before session putting in
j1939_session_deactivate_locked().
Here is the concurrent scenario of the problem reported by syzbot
and my reproduction log.
cpu0 cpu1
j1939_xtp_rx_eoma
j1939_xtp_rx_abort_one
j1939_session_get_by_addr [kref == 2]
j1939_session_get_by_addr [kref == 3]
j1939_session_deactivate [kref == 2]
j1939_session_put [kref == 1]
j1939_session_completed
j1939_session_deactivate
WARN_ON_ONCE(kref < 2)
=====================================================
WARNING: CPU: 1 PID: 21 at net/can/j1939/transport.c:1088 j1939_session_deactivate+0x5f/0x70
CPU: 1 PID: 21 Comm: ksoftirqd/1 Not tainted 5.14.0-rc7+ #32
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014
RIP: 0010:j1939_session_deactivate+0x5f/0x70
Call Trace:
j1939_session_deactivate_activate_next+0x11/0x28
j1939_xtp_rx_eoma+0x12a/0x180
j1939_tp_recv+0x4a2/0x510
j1939_can_recv+0x226/0x380
can_rcv_filter+0xf8/0x220
can_receive+0x102/0x220
? process_backlog+0xf0/0x2c0
can_rcv+0x53/0xf0
__netif_receive_skb_one_core+0x67/0x90
? process_backlog+0x97/0x2c0
__netif_receive_skb+0x22/0x80
Fixes: 0c71437dd50d ("can: j1939: j1939_session_deactivate(): clarify lifetime of session object")
Reported-by: syzbot+9981a614060dcee6eeca@syzkaller.appspotmail.com
Signed-off-by: Ziyang Xuan <redacted>
---
net/can/j1939/transport.c | 4 ----
1 file changed, 4 deletions(-)
@@ -1079,10 +1079,6 @@ static bool j1939_session_deactivate(struct j1939_session *session)boolactive;j1939_session_list_lock(priv);-/* This function should be called with a session ref-count of at-*least2.-*/-WARN_ON_ONCE(kref_read(&session->kref)<2);active=j1939_session_deactivate_locked(session);j1939_session_list_unlock(priv);
Hi,
Thank you for your patches. Please stay on hold, I'll review it end of
this week.
On Mon, Sep 06, 2021 at 05:42:00PM +0800, Ziyang Xuan wrote:
quoted hunk
The conclusion "j1939_session_deactivate() should be called with a
session ref-count of at least 2" is incorrect. In some concurrent
scenarios, j1939_session_deactivate can be called with the session
ref-count less than 2. But there is not any problem because it
will check the session active state before session putting in
j1939_session_deactivate_locked().
Here is the concurrent scenario of the problem reported by syzbot
and my reproduction log.
cpu0 cpu1
j1939_xtp_rx_eoma
j1939_xtp_rx_abort_one
j1939_session_get_by_addr [kref == 2]
j1939_session_get_by_addr [kref == 3]
j1939_session_deactivate [kref == 2]
j1939_session_put [kref == 1]
j1939_session_completed
j1939_session_deactivate
WARN_ON_ONCE(kref < 2)
=====================================================
WARNING: CPU: 1 PID: 21 at net/can/j1939/transport.c:1088 j1939_session_deactivate+0x5f/0x70
CPU: 1 PID: 21 Comm: ksoftirqd/1 Not tainted 5.14.0-rc7+ #32
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014
RIP: 0010:j1939_session_deactivate+0x5f/0x70
Call Trace:
j1939_session_deactivate_activate_next+0x11/0x28
j1939_xtp_rx_eoma+0x12a/0x180
j1939_tp_recv+0x4a2/0x510
j1939_can_recv+0x226/0x380
can_rcv_filter+0xf8/0x220
can_receive+0x102/0x220
? process_backlog+0xf0/0x2c0
can_rcv+0x53/0xf0
__netif_receive_skb_one_core+0x67/0x90
? process_backlog+0x97/0x2c0
__netif_receive_skb+0x22/0x80
Fixes: 0c71437dd50d ("can: j1939: j1939_session_deactivate(): clarify lifetime of session object")
Reported-by: syzbot+9981a614060dcee6eeca@syzkaller.appspotmail.com
Signed-off-by: Ziyang Xuan <redacted>
---
net/can/j1939/transport.c | 4 ----
1 file changed, 4 deletions(-)
@@ -1079,10 +1079,6 @@ static bool j1939_session_deactivate(struct j1939_session *session)boolactive;j1939_session_list_lock(priv);-/* This function should be called with a session ref-count of at-*least2.-*/-WARN_ON_ONCE(kref_read(&session->kref)<2);active=j1939_session_deactivate_locked(session);j1939_session_list_unlock(priv);
On Mon, Sep 06, 2021 at 05:42:00PM +0800, Ziyang Xuan wrote:
The conclusion "j1939_session_deactivate() should be called with a
session ref-count of at least 2" is incorrect. In some concurrent
scenarios, j1939_session_deactivate can be called with the session
ref-count less than 2. But there is not any problem because it
will check the session active state before session putting in
j1939_session_deactivate_locked().
Here is the concurrent scenario of the problem reported by syzbot
and my reproduction log.
cpu0 cpu1
j1939_xtp_rx_eoma
j1939_xtp_rx_abort_one
j1939_session_get_by_addr [kref == 2]
j1939_session_get_by_addr [kref == 3]
j1939_session_deactivate [kref == 2]
j1939_session_put [kref == 1]
j1939_session_completed
j1939_session_deactivate
WARN_ON_ONCE(kref < 2)
Ok, I see, this warning makes sense only if session will actually be
deactivated.
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!
@@ -1079,10 +1079,6 @@ static bool j1939_session_deactivate(struct j1939_session *session)boolactive;j1939_session_list_lock(priv);-/* This function should be called with a session ref-count of at-*least2.-*/-WARN_ON_ONCE(kref_read(&session->kref)<2);active=j1939_session_deactivate_locked(session);j1939_session_list_unlock(priv);
From: Ziyang Xuan (William) <hidden> Date: 2021-11-10 02:00:14
Hello,
I notice that the patch is not applied in upstream. Is it missed
or any other problems?
Thank you!
On Mon, Sep 06, 2021 at 05:42:00PM +0800, Ziyang Xuan wrote:
quoted
The conclusion "j1939_session_deactivate() should be called with a
session ref-count of at least 2" is incorrect. In some concurrent
scenarios, j1939_session_deactivate can be called with the session
ref-count less than 2. But there is not any problem because it
will check the session active state before session putting in
j1939_session_deactivate_locked().
Here is the concurrent scenario of the problem reported by syzbot
and my reproduction log.
cpu0 cpu1
j1939_xtp_rx_eoma
j1939_xtp_rx_abort_one
j1939_session_get_by_addr [kref == 2]
j1939_session_get_by_addr [kref == 3]
j1939_session_deactivate [kref == 2]
j1939_session_put [kref == 1]
j1939_session_completed
j1939_session_deactivate
WARN_ON_ONCE(kref < 2)
Ok, I see, this warning makes sense only if session will actually be
deactivated.
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!
@@ -1079,10 +1079,6 @@ static bool j1939_session_deactivate(struct j1939_session *session)boolactive;j1939_session_list_lock(priv);-/* This function should be called with a session ref-count of at-*least2.-*/-WARN_ON_ONCE(kref_read(&session->kref)<2);active=j1939_session_deactivate_locked(session);j1939_session_list_unlock(priv);
Ok, I see, this warning makes sense only if session will actually be
deactivated.
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Thank you!
As Ziyang Xuan stated, the patch was not applied to upstream.
Usage of WARN_ON_ONCE in this case is actually discouraged: it erroneusly
complains in a valid situation.
So the macro should be removed with the aforementioned patch. If it makes
some sense for debugging purposes, WARN_ON_ONCE can be replaced with
netdev_warn/netdev_notice but anyway discard of WARN_ON_ONCE.
--
Regards,
Fedor