Skip to contentSkip to conversation

can: protect the receive lists with a per-device lock

PATCH Superseded Oliver Hartkopp 3 participants 13 messages Oct 19

Subsystems (6)
Networking Stable branch Documentation CAN network drivers +2 more subsystems

v3 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

v3 cover can: protect the receive lists with a per-device lock

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next with the linux-can-next pull request [2]. They are included unchanged only to provide a sequence that applies on top of mainline, so that the sashiko bot is able to build and review patch 4. Please do not apply patches 1-3 from this series.

The idea behind patch 4:

The receive lists of a CAN device are located in the device's can_ml_priv and travel with the device when it is moved to another network namespace. But they are protected by the per-netns rcvlists_lock, so the lock that protects a device's lists depends on the namespace the caller passes to can_rx_register() and can_rx_unregister(). can_rx_unregister() silently returns when this namespace does not match dev_net(dev) anymore.

isotp_release() and bcm_release() leave the netdev notifier list before they can serialize against the notifier. When the device changes its namespace in this window, their filters are not removed and the remaining receivers point to the freed socket or bcm op.

Taking RTNL in these release paths is no option, as CAN_RAW already holds RTNL while adding and removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged users can trigger repeatedly.

Instead, patch 4 protects the receive lists where they are located: A spinlock in struct can_dev_rcv_lists protects the lists of a CAN device (initialized in can_set_ml_priv()) or the 'any' CAN device lists of a namespace (initialized in can_pernet_init()). can_rx_unregister() then finds the subscription independently of the current namespace of the device, which is kept accessible by the tracked device references the protocols already hold. The per-netns rcvlists_lock only protects the receive list statistics, for which the callers pass the namespace that has been used for can_rx_register().

As a consequence, CAN_RAW and CAN_ISOTP do not need RTNL in bind(), setsockopt() and release() anymore. bind() is serialized against NETDEV_UNREGISTER by publishing the device inside the RCU section of its lookup, as unlist_netdevice() and synchronize_net() precede NETDEV_UNREGISTER both on unregistration and on a namespace change. The notifiers recheck the binding under lock_sock().

[1] https://lore.kernel.org/linux-can/[email protected]/
[2] https://lore.kernel.org/linux-can/[email protected]/

Previous versions of patch 4:
v2: https://lore.kernel.org/linux-can/[email protected]/
v1: https://lore.kernel.org/linux-can/[email protected]/

Oliver Hartkopp (3):
can: proc: remove pointers from CAN specific proc output
can: replace ARPHRD_CAN type checks with can_get_ml_priv()
can: protect the receive lists with a per-device lock

Runyu Xiao (1):
can: proc: reset pkg_stats atomics individually

 Documentation/networking/can.rst |  16 ++--
 include/linux/can/can-ml.h       |   3 +
 include/linux/can/core.h         |   2 +-
 include/net/netns/can.h          |   2 +-
 net/can/af_can.c                 |  59 ++++++------
 net/can/af_can.h                 |   2 +-
 net/can/bcm.c                    |  64 +++++--------
 net/can/gw.c                     |   9 +-
 net/can/isotp.c                  |  49 ++++++----
 net/can/j1939/main.c             |   2 +-
 net/can/proc.c                   |  65 +++++++++-----
 net/can/raw.c                    | 150 ++++++++++++++++++++-----------
 12 files changed, 252 insertions(+), 171 deletions(-)

base-commit: 32c52ec87d6245ee2d12ed5dd509e40062939b50

Signature · 1 line
2.43.0
author maintainer
#1 lore raw

PATCH v3 1/4 can: proc: reset pkg_stats atomics individually

From: Runyu Xiao <[email protected]>

Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path") converted several members of struct can_pkg_stats to atomic_long_t and updated the hot TX/RX and procfs read paths to use atomic_long_*() helpers. However, can_init_stats() still clears the whole struct with memset().

can_init_stats() is reached from can_stat_update() in timer context and also from the procfs reset path. Those paths can run while the TX/RX hot paths are concurrently updating rx_frames, tx_frames, matches and their *_delta counters. Hitting the whole-struct memset() in that window performs plain writes to fields that otherwise follow an atomic_long_t access contract, which can lose or mix live statistics updates.

This issue was found by source-level API-misuse analysis looking for whole-object resets left behind after atomic_long_t conversions, then manually audited on Linux v6.18.21.

Replace the whole-struct memset() with a helper that resets the atomic_long_t counters via atomic_long_set() and clears the derived scalar statistics explicitly. This preserves the existing reset semantics for scalar fields while restoring atomic access discipline for the live counters.

Build-tested by compiling net/can/proc.o on x86_64 netdev/main. Runtime-tested with a QEMU + vcan setup on Linux v6.18.21 by driving concurrent traffic and reset_stats reads, which reproduced inconsistent exported statistics before the fix.

[Hartkopp] can_stat_update() also read the *_delta counters at the beginning and cleared them with atomic_long_set() at the end. Increments from the hot path in between were lost for both the current and the next interval. Use atomic_long_xchg() to fetch and clear the *_delta counters in one atomic step instead.

Fixes 80b5f90158d1 ("can: statistics: use atomic access in hot path")
Cc [email protected]
Signed-off-by Runyu Xiao <[email protected]>
Acked-by Oliver Hartkopp <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/proc.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)
net/can/proc.c +29 -10
index de4d05ae3459..70104f13077e 100644
@@ -74,20 +74,43 @@ static const char rx_list_name[][8] = {
 
 /*
  * af_can statistics stuff
  */
 
+static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
+{
+	atomic_long_set(&pkg_stats->rx_frames, 0);
+	atomic_long_set(&pkg_stats->tx_frames, 0);
+	atomic_long_set(&pkg_stats->matches, 0);
+
+	pkg_stats->total_rx_rate = 0;
+	pkg_stats->total_tx_rate = 0;
+	pkg_stats->total_rx_match_ratio = 0;
+
+	pkg_stats->current_rx_rate = 0;
+	pkg_stats->current_tx_rate = 0;
+	pkg_stats->current_rx_match_ratio = 0;
+
+	pkg_stats->max_rx_rate = 0;
+	pkg_stats->max_tx_rate = 0;
+	pkg_stats->max_rx_match_ratio = 0;
+
+	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->matches_delta, 0);
+}
+
 static void can_init_stats(struct net *net)
 {
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	/*
-	 * This memset function is called from a timer context (when
+	 * This stats reset is called from a timer context (when
 	 * can_stattimer is active which is the default) OR in a process
 	 * context (reading the proc_fs when can_stattimer is disabled).
 	 */
-	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
+	can_reset_pkg_stats(pkg_stats);
 	pkg_stats->jiffies_init = jiffies;
 
 	rcv_lists_stats->stats_reset++;
 
 	if (user_reset) {

@@ -119,13 +142,14 @@ void can_stat_update(struct timer_list *t)
 	unsigned long j = jiffies; /* snapshot */
 
 	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
 	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
 	long matches = atomic_long_read(&pkg_stats->matches);
-	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);
-	long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta);
-	long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+	/* fetch and clear values for 'current rate' calculation */
+	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
+	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
+	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
 
 	/* restart counting in timer context on user request */
 	if (user_reset)
 		can_init_stats(net);
 

@@ -170,15 +194,10 @@ void can_stat_update(struct timer_list *t)
 		pkg_stats->max_rx_rate = pkg_stats->current_rx_rate;
 
 	if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio)
 		pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio;
 
-	/* clear values for 'current rate' calculation */
-	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
-	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
-	atomic_long_set(&pkg_stats->matches_delta, 0);
-
 	/* restart timer (one second) */
 	mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ));
 }
 
 /*
Signature · 1 line
2.43.0
AI review
#2 lore raw

Re: [PATCH can-next v3 1/4] can: proc: reset pkg_stats atomics individually

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] can: proc: stale counter snapshots in can_stat_update() overwrite reset stats --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: proc: stale counter snapshots in can_stat_update() overwrite reset stats --

commit 947871fd730bb7740551ee6c8b42c7ac5ed61308
Author: Runyu Xiao <[email protected]>

can: proc: reset pkg_stats atomics individually

This patch replaces the whole-struct memset() in can_init_stats() with individual atomic_long_set() and scalar field resets in can_reset_pkg_stats(), and switches can_stat_update() to use atomic_long_xchg() when reading and clearing the delta counters.

Quoted 50 lines
net/can/proc.c +25 -2
index de4d05ae34597..70104f13077e0 100644
@@ -76,16 +76,39 @@ static const char rx_list_name[][8] = {
  * af_can statistics stuff
  */
 
+static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
+{
+	atomic_long_set(&pkg_stats->rx_frames, 0);
+	atomic_long_set(&pkg_stats->tx_frames, 0);
+	atomic_long_set(&pkg_stats->matches, 0);
+
+	pkg_stats->total_rx_rate = 0;
+	pkg_stats->total_tx_rate = 0;
+	pkg_stats->total_rx_match_ratio = 0;
+
+	pkg_stats->current_rx_rate = 0;
+	pkg_stats->current_tx_rate = 0;
+	pkg_stats->current_rx_match_ratio = 0;
+
+	pkg_stats->max_rx_rate = 0;
+	pkg_stats->max_tx_rate = 0;
+	pkg_stats->max_rx_match_ratio = 0;
+
+	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->matches_delta, 0);
+}
+
 static void can_init_stats(struct net *net)
 {
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	/*
-	 * This memset function is called from a timer context (when
+	 * This stats reset is called from a timer context (when
 	 * can_stattimer is active which is the default) OR in a process
 	 * context (reading the proc_fs when can_stattimer is disabled).
 	 */
-	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
+	can_reset_pkg_stats(pkg_stats);
 	pkg_stats->jiffies_init = jiffies;
 
 	rcv_lists_stats->stats_reset++;

@@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t)
 	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
 	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
 	long matches = atomic_long_read(&pkg_stats->matches);

net/can/proc.c void can_stat_update(struct timer_list *t) · line 153

Quoted 10 lines
net/can/proc.c +4 -3
-	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);
-	long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta);
-	long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+	/* fetch and clear values for 'current rate' calculation */
+	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
+	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
+	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
 
 	/* restart counting in timer context on user request */
 	if (user_reset)

[Severity: Medium]
Does can_stat_update() overwrite the freshly reset scalar statistics in pkg_stats with stale pre-reset counter snapshots after calling can_init_stats()?

At the beginning of can_stat_update() in net/can/proc.c, the counters are snapshotted into local variables before checking for a reset or overflow:

	unsigned long j = jiffies; /* snapshot */

	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
	long matches = atomic_long_read(&pkg_stats->matches);
	/* fetch and clear values for 'current rate' calculation */
	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);

	/* restart counting in timer context on user request */
	if (user_reset)
		can_init_stats(net);

	/* restart counting on jiffies overflow */
	if (j < pkg_stats->jiffies_init)
		can_init_stats(net);

	/* prevent overflow in calc_rate() */
	if (rx_frames > (LONG_MAX / HZ))
		can_init_stats(net);
	...

When user_reset is set (for example, after reading /proc/net/can/reset_stats) or when one of the overflow conditions triggers, can_stat_update() calls can_init_stats() -> can_reset_pkg_stats(), which zeroes the atomic counters and the derived scalar fields (total_*, current_*, max_*).

However, the local variables rx_frames, tx_frames, matches, rx_frames_delta, tx_frames_delta, and matches_delta still hold the pre-reset values when can_stat_update() continues immediately below:

	/* calc total values */
	if (rx_frames)
		pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;

	pkg_stats->total_tx_rate = calc_rate(pkg_stats->jiffies_init, j,
					    tx_frames);
	pkg_stats->total_rx_rate = calc_rate(pkg_stats->jiffies_init, j,
					    rx_frames);

	/* calc current values */
	if (rx_frames_delta)
		pkg_stats->current_rx_match_ratio =
			(matches_delta * 100) /	rx_frames_delta;

	pkg_stats->current_tx_rate = calc_rate(0, HZ, tx_frames_delta);
	pkg_stats->current_rx_rate = calc_rate(0, HZ, rx_frames_delta);

	/* check / update maximum values */
	if (pkg_stats->max_tx_rate < pkg_stats->current_tx_rate)
		pkg_stats->max_tx_rate = pkg_stats->current_tx_rate;
	...

As a result, those pre-reset local variables overwrite total_rx_match_ratio, current_*, and max_* right after can_reset_pkg_stats() cleared them (and if the reset was triggered by the rx_frames, tx_frames, or matches overflow checks, the overflowing values are still passed into the ratio and calc_rate() calculations).

Additionally, because can_init_stats() sets pkg_stats->jiffies_init = jiffies by reading live jiffies rather than using the snapshot j, if jiffies ticks between the snapshot and can_init_stats(net) during a user_reset, the subsequent check (j < pkg_stats->jiffies_init) will also evaluate to true, calling can_init_stats(net) a second time and then passing jiffies_init > j along with the stale non-zero tx_frames and rx_frames into calc_rate().

Should can_stat_update() re-arm the timer and return early (or zero the local snapshot variables) when can_init_stats() is called?

[ ... ]

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

Re: [PATCH can-next v3 1/4] can: proc: reset pkg_stats atomics individually

Quoted 72 lines · On 19.10.26 09:53, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: proc: stale counter snapshots in can_stat_update() overwrite reset stats --

commit 947871fd730bb7740551ee6c8b42c7ac5ed61308
Author: Runyu Xiao <[email protected]>

can: proc: reset pkg_stats atomics individually

This patch replaces the whole-struct memset() in can_init_stats() with individual atomic_long_set() and scalar field resets in can_reset_pkg_stats(), and switches can_stat_update() to use atomic_long_xchg() when reading and clearing the delta counters.

Quoted 58 lines
net/can/proc.c +29 -5
index de4d05ae34597..70104f13077e0 100644
@@ -76,16 +76,39 @@ static const char rx_list_name[][8] = {
   * af_can statistics stuff
   */
 
+static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
+{
+	atomic_long_set(&pkg_stats->rx_frames, 0);
+	atomic_long_set(&pkg_stats->tx_frames, 0);
+	atomic_long_set(&pkg_stats->matches, 0);
+
+	pkg_stats->total_rx_rate = 0;
+	pkg_stats->total_tx_rate = 0;
+	pkg_stats->total_rx_match_ratio = 0;
+
+	pkg_stats->current_rx_rate = 0;
+	pkg_stats->current_tx_rate = 0;
+	pkg_stats->current_rx_match_ratio = 0;
+
+	pkg_stats->max_rx_rate = 0;
+	pkg_stats->max_tx_rate = 0;
+	pkg_stats->max_rx_match_ratio = 0;
+
+	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->matches_delta, 0);
+}
+
  static void can_init_stats(struct net *net)
  {
  	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
  	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
  	/*
-	 * This memset function is called from a timer context (when
+	 * This stats reset is called from a timer context (when
  	 * can_stattimer is active which is the default) OR in a process
  	 * context (reading the proc_fs when can_stattimer is disabled).
  	 */
-	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
+	can_reset_pkg_stats(pkg_stats);
  	pkg_stats->jiffies_init = jiffies;
 
  	rcv_lists_stats->stats_reset++;

@@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t)
  	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
  	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
  	long matches = atomic_long_read(&pkg_stats->matches);
-	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);
-	long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta);
-	long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+	/* fetch and clear values for 'current rate' calculation */
+	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
+	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
+	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
Quoted 7 lines

net/can/proc.c void can_stat_update(struct timer_list *t) · line 153

Quoted 2 lines
net/can/proc.c +0 -0
  	/* restart counting in timer context on user request */
  	if (user_reset)

[Severity: Medium]
Does can_stat_update() overwrite the freshly reset scalar statistics in pkg_stats with stale pre-reset counter snapshots after calling can_init_stats()?

Yes, but this is not introduced by this patch. The local snapshots were added by 80b5f90158d1 (the commit this patch fixes) and this patch does not change the resulting values.

I would rate this as Low, as the impact is limited to cosmetic procfs statistics:

- total_rx_match_ratio keeps its pre-reset value until new frames are received.
- current_* and max_* show the last pre-reset interval for one timer period (one second).
- A jiffies tick between snapshot and reset only increments stats_reset twice; calc_rate() returns 0 in this case, no crash.

I'll address this in a separate patch with the same Fixes: tag by returning early from can_stat_update() after a reset.

Best regards,
Oliver

pw-bot: cr

Quoted 76 lines

At the beginning of can_stat_update() in net/can/proc.c, the counters are snapshotted into local variables before checking for a reset or overflow:

	unsigned long j = jiffies; /* snapshot */

	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
	long matches = atomic_long_read(&pkg_stats->matches);
	/* fetch and clear values for 'current rate' calculation */
	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);

	/* restart counting in timer context on user request */
	if (user_reset)
		can_init_stats(net);

	/* restart counting on jiffies overflow */
	if (j < pkg_stats->jiffies_init)
		can_init_stats(net);

	/* prevent overflow in calc_rate() */
	if (rx_frames > (LONG_MAX / HZ))
		can_init_stats(net);
	...

When user_reset is set (for example, after reading /proc/net/can/reset_stats) or when one of the overflow conditions triggers, can_stat_update() calls can_init_stats() -> can_reset_pkg_stats(), which zeroes the atomic counters and the derived scalar fields (total_*, current_*, max_*).

However, the local variables rx_frames, tx_frames, matches, rx_frames_delta, tx_frames_delta, and matches_delta still hold the pre-reset values when can_stat_update() continues immediately below:

	/* calc total values */
	if (rx_frames)
		pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;

	pkg_stats->total_tx_rate = calc_rate(pkg_stats->jiffies_init, j,
					    tx_frames);
	pkg_stats->total_rx_rate = calc_rate(pkg_stats->jiffies_init, j,
					    rx_frames);

	/* calc current values */
	if (rx_frames_delta)
		pkg_stats->current_rx_match_ratio =
			(matches_delta * 100) /	rx_frames_delta;

	pkg_stats->current_tx_rate = calc_rate(0, HZ, tx_frames_delta);
	pkg_stats->current_rx_rate = calc_rate(0, HZ, rx_frames_delta);

	/* check / update maximum values */
	if (pkg_stats->max_tx_rate < pkg_stats->current_tx_rate)
		pkg_stats->max_tx_rate = pkg_stats->current_tx_rate;
	...

As a result, those pre-reset local variables overwrite total_rx_match_ratio, current_*, and max_* right after can_reset_pkg_stats() cleared them (and if the reset was triggered by the rx_frames, tx_frames, or matches overflow checks, the overflowing values are still passed into the ratio and calc_rate() calculations).

Additionally, because can_init_stats() sets pkg_stats->jiffies_init = jiffies by reading live jiffies rather than using the snapshot j, if jiffies ticks between the snapshot and can_init_stats(net) during a user_reset, the subsequent check (j < pkg_stats->jiffies_init) will also evaluate to true, calling can_init_stats(net) a second time and then passing jiffies_init > j along with the stale non-zero tx_frames and rx_frames into calc_rate().

Should can_stat_update() re-arm the timer and return early (or zero the local snapshot variables) when can_init_stats() is called?

[ ... ]

author maintainer
#4 lore raw

Re: [PATCH can-next v3 1/4] can: proc: reset pkg_stats atomics individually

I sent a patch that fixes this sashiko-bot review remark:

https://lore.kernel.org/linux-can/[email protected]/

Best regards,
Oliver

pw-bot: cr

Quoted 189 lines · Oliver Hartkopp
Quoted 83 lines · On 19.10.26 09:53, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: proc: stale counter snapshots in can_stat_update() overwrite reset stats

Signature · 9 lines
commit 947871fd730bb7740551ee6c8b42c7ac5ed61308
Author: Runyu Xiao <[email protected]>

can: proc: reset pkg_stats atomics individually

This patch replaces the whole-struct memset() in can_init_stats() with
individual atomic_long_set() and scalar field resets in can_reset_pkg_stats(),
and switches can_stat_update() to use atomic_long_xchg() when reading and
clearing the delta counters.
Quoted 59 lines
net/can/proc.c +23 -0
index de4d05ae34597..70104f13077e0 100644
@@ -76,16 +76,39 @@ static const char rx_list_name[][8] = {
    * af_can statistics stuff
    */
+static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
+{
+    atomic_long_set(&pkg_stats->rx_frames, 0);
+    atomic_long_set(&pkg_stats->tx_frames, 0);
+    atomic_long_set(&pkg_stats->matches, 0);
+
+    pkg_stats->total_rx_rate = 0;
+    pkg_stats->total_tx_rate = 0;
+    pkg_stats->total_rx_match_ratio = 0;
+
+    pkg_stats->current_rx_rate = 0;
+    pkg_stats->current_tx_rate = 0;
+    pkg_stats->current_rx_match_ratio = 0;
+
+    pkg_stats->max_rx_rate = 0;
+    pkg_stats->max_tx_rate = 0;
+    pkg_stats->max_rx_match_ratio = 0;
+
+    atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+    atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+    atomic_long_set(&pkg_stats->matches_delta, 0);
+}
+
   static void can_init_stats(struct net *net)
   {
       struct can_pkg_stats *pkg_stats = net->can.pkg_stats;

      struct can_rcv_lists_stats *rcv_lists_stats = net-

Quoted 1 line

can.rcv_lists_stats;

      /*
-     * This memset function is called from a timer context (when +     * This stats reset is called from a timer context (when
       * can_stattimer is active which is the default) OR in a process
       * context (reading the proc_fs when can_stattimer is disabled). */
-    memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
+    can_reset_pkg_stats(pkg_stats); pkg_stats->jiffies_init = jiffies; rcv_lists_stats->stats_reset++;

net/can/proc.c +3 -3
@@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t)
       long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
       long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
       long matches = atomic_long_read(&pkg_stats->matches);
-    long rx_frames_delta = atomic_long_read(&pkg_stats- 
 >rx_frames_delta);
-    long tx_frames_delta = atomic_long_read(&pkg_stats- 
 >tx_frames_delta);
-    long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+    /* fetch and clear values for 'current rate' calculation */
+    long rx_frames_delta = atomic_long_xchg(&pkg_stats- 
 >rx_frames_delta, 0);
+    long tx_frames_delta = atomic_long_xchg(&pkg_stats-
Quoted 1 line

tx_frames_delta, 0);

Quoted 4 lines

+    long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
      /* restart counting in timer context on user request */ if (user_reset)

[Severity: Medium]
Does can_stat_update() overwrite the freshly reset scalar statistics in pkg_stats with stale pre-reset counter snapshots after calling can_init_stats()?

Yes, but this is not introduced by this patch. The local snapshots were added by 80b5f90158d1 (the commit this patch fixes) and this patch does not change the resulting values.

I would rate this as Low, as the impact is limited to cosmetic procfs statistics:

- total_rx_match_ratio keeps its pre-reset value until new frames are received.
- current_* and max_* show the last pre-reset interval for one timer period (one second).
- A jiffies tick between snapshot and reset only increments stats_reset twice; calc_rate() returns 0 in this case, no crash.

I'll address this in a separate patch with the same Fixes: tag by returning early from can_stat_update() after a reset.

Best regards,
Oliver

pw-bot: cr

Quoted 78 lines

At the beginning of can_stat_update() in net/can/proc.c, the counters are snapshotted into local variables before checking for a reset or overflow:

    unsigned long j = jiffies; /* snapshot */

    long rx_frames = atomic_long_read(&pkg_stats->rx_frames); long tx_frames = atomic_long_read(&pkg_stats->tx_frames); long matches = atomic_long_read(&pkg_stats->matches); /* fetch and clear values for 'current rate' calculation */ long rx_frames_delta = atomic_long_xchg(&pkg_stats-

Quoted 1 line

rx_frames_delta, 0);

    long tx_frames_delta = atomic_long_xchg(&pkg_stats-

Quoted 1 line

tx_frames_delta, 0);

    long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);

    /* restart counting in timer context on user request */ if (user_reset)
        can_init_stats(net);

    /* restart counting on jiffies overflow */
    if (j < pkg_stats->jiffies_init)
        can_init_stats(net);

    /* prevent overflow in calc_rate() */
    if (rx_frames > (LONG_MAX / HZ))
        can_init_stats(net);
    ...

When user_reset is set (for example, after reading /proc/net/can/ reset_stats) or when one of the overflow conditions triggers, can_stat_update() calls can_init_stats() -> can_reset_pkg_stats(), which zeroes the atomic counters and the derived scalar fields (total_*, current_*, max_*).

However, the local variables rx_frames, tx_frames, matches, rx_frames_delta, tx_frames_delta, and matches_delta still hold the pre-reset values when can_stat_update() continues immediately below:

    /* calc total values */
    if (rx_frames)
        pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;

    pkg_stats->total_tx_rate = calc_rate(pkg_stats->jiffies_init, j, tx_frames); pkg_stats->total_rx_rate = calc_rate(pkg_stats->jiffies_init, j, rx_frames);

    /* calc current values */
    if (rx_frames_delta) pkg_stats->current_rx_match_ratio = (matches_delta * 100) /    rx_frames_delta;

    pkg_stats->current_tx_rate = calc_rate(0, HZ, tx_frames_delta); pkg_stats->current_rx_rate = calc_rate(0, HZ, rx_frames_delta);

    /* check / update maximum values */
    if (pkg_stats->max_tx_rate < pkg_stats->current_tx_rate) pkg_stats->max_tx_rate = pkg_stats->current_tx_rate;
    ...

As a result, those pre-reset local variables overwrite total_rx_match_ratio, current_*, and max_* right after can_reset_pkg_stats() cleared them (and if the reset was triggered by the rx_frames, tx_frames, or matches overflow checks, the overflowing values are still passed into the ratio and calc_rate() calculations).

Additionally, because can_init_stats() sets pkg_stats->jiffies_init = jiffies by reading live jiffies rather than using the snapshot j, if jiffies ticks between the snapshot and can_init_stats(net) during a user_reset, the subsequent check (j < pkg_stats->jiffies_init) will also evaluate to true, calling can_init_stats(net) a second time and then passing jiffies_init > j along with the stale non-zero tx_frames and rx_frames into calc_rate().

Should can_stat_update() re-arm the timer and return early (or zero the local snapshot variables) when can_init_stats() is called?

[ ... ]

author maintainer
#5 lore raw

PATCH v3 2/4 can: proc: remove pointers from CAN specific proc output

In reply to Oliver Hartkopp

The proc content in /proc/net/can/ and /proc/net/can-bcm/ is intended to check the internal filter lists (af_can, can_raw) and the efficiency and functionality of can_bcm jobs. While exposing kernel addresses was acceptable when this code was written the times have changed and multiple attempts have been made to hash or remove such now sensitive data.

This patch removes the disclosure of pointers and instead provides the function names and sock inode numbers when available. When the function name can not be resolved (e.g. KALLSYMS=n or stripped module symbols) %ps would fall back to the raw text address. Therefore (unknown) is printed instead in this case. As there's no known tooling around the CAN specific proc output breaking the ABI with this rework creates no issue.

Suggested-by Sebastian Andrzej Siewior <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
 Documentation/networking/can.rst | 16 ++++++++++------
 include/linux/can/core.h         |  2 +-
 net/can/af_can.c                 |  6 +++---
 net/can/af_can.h                 |  2 +-
 net/can/bcm.c                    | 14 +++++++-------
 net/can/gw.c                     |  2 +-
 net/can/isotp.c                  |  4 ++--
 net/can/j1939/main.c             |  2 +-
 net/can/proc.c                   | 26 ++++++++++++++++----------
 net/can/raw.c                    |  4 ++--
 10 files changed, 44 insertions(+), 34 deletions(-)
Documentation/networking/can.rst +10 -6
index 536ff411da1d..51bc70748f75 100644
@@ -1038,21 +1038,25 @@ procfs content
 
 As described in :ref:`socketcan-receive-lists` the SocketCAN core uses several filter
 lists to deliver received CAN frames to CAN protocol modules. These
 receive lists, their filters and the count of filter matches can be
 checked in the appropriate receive list. All entries contain the
-device and a protocol module identifier::
+CAN network device, the filter (can_id/can_mask), the number of filter
+matches, the inode number of the subscribing socket and the name of the
+receive function. A sock_inode of 0 denotes a kernel internal subscriber
+without a socket (e.g. CAN gateway or J1939). When the function name can
+not be resolved (e.g. with CONFIG_KALLSYMS=n) "(unknown)" is printed::
 
     foo@bar:~$ cat /proc/net/can/rcvlist_all
 
     receive list 'rx_all':
-      (vcan3: no entry)
-      (vcan2: no entry)
-      (vcan1: no entry)
-      device   can_id   can_mask  function  userdata   matches  ident
-       vcan0     000    00000000  f88e6370  f6c6f400         0  raw
       (any: no entry)
+      device   can_id   can_mask   matches  sock_inode  function
+       vcan0     000    00000000         0       63586  raw_rcv [can_raw]
+      (vcan1: no entry)
+      (vcan2: no entry)
+      (vcan3: no entry)
 
 In this example an application requests any CAN traffic from vcan0::
 
     rcvlist_all - list for unfiltered entries (no filter operations)
     rcvlist_eff - list for single extended frame (EFF) entries
include/linux/can/core.h +1 -1
index 2de74c2b78b6..effb5c31ef40 100644
@@ -49,11 +49,11 @@ extern int  can_proto_register(const struct can_proto *cp);
 extern void can_proto_unregister(const struct can_proto *cp);
 
 int can_rx_register(struct net *net, struct net_device *dev,
 		    canid_t can_id, canid_t mask,
 		    void (*func)(struct sk_buff *, void *),
-		    void *data, char *ident, struct sock *sk);
+		    void *data, u64 ino, struct sock *sk);
 
 extern void can_rx_unregister(struct net *net, struct net_device *dev,
 			      canid_t can_id, canid_t mask,
 			      void (*func)(struct sk_buff *, void *),
 			      void *data);
net/can/af_can.c +3 -3
index 34fe3b28d576..6ebe9d3689ac 100644
@@ -416,11 +416,11 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
  * @dev: pointer to netdevice (NULL => subscribe from 'all' CAN devices list)
  * @can_id: CAN identifier (see description)
  * @mask: CAN mask (see description)
  * @func: callback function on filter match
  * @data: returned parameter for callback function
- * @ident: string for calling module identification
+ * @ino: inode number of sock (0 = unknown)
  * @sk: socket pointer (might be NULL)
  *
  * Description:
  *  Invokes the callback function with the received sk_buff and the given
  *  parameter 'data' on a matching receive filter. A filter matches, when

@@ -441,11 +441,11 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
  *  -ENOMEM on missing cache mem to create subscription entry
  *  -ENODEV unknown device
  */
 int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 		    canid_t mask, void (*func)(struct sk_buff *, void *),
-		    void *data, char *ident, struct sock *sk)
+		    void *data, u64 ino, struct sock *sk)
 {
 	struct receiver *rcv;
 	struct hlist_head *rcv_list;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;

@@ -470,11 +470,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	rcv->can_id = can_id;
 	rcv->mask = mask;
 	atomic_long_set(&rcv->matches, 0);
 	rcv->func = func;
 	rcv->data = data;
-	rcv->ident = ident;
+	rcv->ino = ino;
 	rcv->sk = sk;
 
 	hlist_add_head_rcu(&rcv->list, rcv_list);
 	dev_rcv_lists->entries++;
net/can/af_can.h +1 -1
index 87887014f562..6e593ed5db5f 100644
@@ -53,11 +53,11 @@ struct receiver {
 	canid_t can_id;
 	canid_t mask;
 	atomic_long_t matches;
 	void (*func)(struct sk_buff *skb, void *data);
 	void *data;
-	char *ident;
+	u64 ino;
 	struct sock *sk;
 	struct rcu_head rcu;
 };
 
 /* statistic structures */
net/can/bcm.c +7 -7
index 3d637a1e0ac1..6c85a76477c3 100644
@@ -218,13 +218,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 	struct net *net = m->private;
 	struct sock *sk = (struct sock *)pde_data(m->file->f_inode);
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 
-	seq_printf(m, ">>> socket %pK", sk->sk_socket);
-	seq_printf(m, " / sk %pK", sk);
-	seq_printf(m, " / bo %pK", bo);
+	seq_printf(m, ">>> sock inode %s", bo->procname);
 	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
 	seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));
 	seq_printf(m, " <<<\n");
 
 	rcu_read_lock();

@@ -1534,11 +1532,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			if (dev) {
 				err = can_rx_register(sock_net(sk), dev,
 						      op->can_id,
 						      REGMASK(op->can_id),
 						      bcm_rx_handler, op,
-						      "bcm", sk);
+						      sock_i_ino(sk), sk);
 
 				/* keep a tracked reference so that a later
 				 * unregister can safely reach the device even
 				 * if a concurrent NETDEV_UNREGISTER has
 				 * already unlisted it by ifindex

@@ -1558,11 +1556,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			}
 
 		} else {
 			err = can_rx_register(sock_net(sk), NULL, op->can_id,
 					      REGMASK(op->can_id),
-					      bcm_rx_handler, op, "bcm", sk);
+					      bcm_rx_handler, op,
+					      sock_i_ino(sk), sk);
 		}
 
 		if (err) {
 			/* newly created bcm rx op is broken -> remove it */
 			if (new_op) {

@@ -2038,12 +2037,13 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 		bo->ifindex = 0;
 	}
 
 #if IS_ENABLED(CONFIG_PROC_FS)
 	if (net->can.bcmproc_dir) {
-		/* unique socket address as filename */
-		sprintf(bo->procname, "%llu", sock_i_ino(sk));
+		/* use unique socket inode number as filename */
+		snprintf(bo->procname, sizeof(bo->procname),
+			 "%llu", sock_i_ino(sk));
 		bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644,
 						     net->can.bcmproc_dir,
 						     bcm_proc_show, sk);
 		if (!bo->bcm_proc_read) {
 			ret = -ENOMEM;
net/can/gw.c +1 -1
index 5793cb2420ed..36f9aa0f7026 100644
@@ -577,11 +577,11 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 
 static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
 {
 	return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
 			       gwj->ccgw.filter.can_mask, can_can_gw_rcv,
-			       gwj, "gw", NULL);
+			       gwj, 0, NULL);
 }
 
 static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
 {
 	can_rx_unregister(net, gwj->src.dev, gwj->ccgw.filter.can_id,
net/can/isotp.c +2 -2
index f5dc9d04bd68..b239f4b45518 100644
@@ -1623,18 +1623,18 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	ifindex = dev->ifindex;
 
 	if (isotp_register_rxid(so))
 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
-				isotp_rcv, sk, "isotp", sk);
+				isotp_rcv, sk, sock_i_ino(sk), sk);
 
 	/* no consecutive frame echo skb in flight */
 	WRITE_ONCE(so->cfecho, 0);
 
 	/* register for echo skb's */
 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
-			isotp_rcv_echo, sk, "isotpe", sk);
+			isotp_rcv_echo, sk, sock_i_ino(sk), sk);
 
 	/* switch to new settings */
 	so->ifindex = ifindex;
 	so->rxid = rx_id;
 	so->txid = tx_id;
net/can/j1939/main.c +1 -1
index 5e5e6c228f22..d9384cf0e356 100644
@@ -182,11 +182,11 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
 	struct net_device *ndev = priv->ndev;
 	int ret;
 
 	j1939_priv_get(priv);
 	ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
-			      j1939_can_recv, priv, "j1939", NULL);
+			      j1939_can_recv, priv, 0, NULL);
 	if (ret < 0) {
 		j1939_priv_put(priv);
 		return ret;
 	}
net/can/proc.c +16 -10
index 70104f13077e..0f1fa6564436 100644
@@ -208,30 +208,36 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
 			      struct net_device *dev)
 {
 	struct receiver *r;
 
 	hlist_for_each_entry_rcu(r, rx_list, list) {
+		char symcheck[4];
 		char *fmt = (r->can_id & CAN_EFF_FLAG)?
-			"   %-5s  %08x  %08x  %pK  %pK  %8ld  %s\n" :
-			"   %-5s     %03x    %08x  %pK  %pK  %8ld  %s\n";
+			"  %6s  %08x  %08x  %8ld  %10llu" :
+			"  %6s     %03x    %08x  %8ld  %10llu";
 
 		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
-			   r->func, r->data, atomic_long_read(&r->matches),
-			   r->ident);
+			   atomic_long_read(&r->matches), r->ino);
+
+		/* %ps falls back to the raw text address when the symbol
+		 * can not be resolved (KALLSYMS=n, stripped module symbols)
+		 */
+		snprintf(symcheck, sizeof(symcheck), "%ps", r->func);
+		if (strncmp(symcheck, "0x", 2))
+			seq_printf(m, "  %ps\n", r->func);
+		else
+			seq_puts(m, "  (unknown)\n");
 	}
 }
 
 static void can_print_recv_banner(struct seq_file *m)
 {
 	/*
-	 *                  can1.  00000000  00000000  00000000
-	 *                 .......          0  tp20
+	 *    device   can_id   can_mask   matches  sock_inode  function
+	 *     vcan0     000    00000000         0       43798  raw_rcv [can_raw]
 	 */
-	if (IS_ENABLED(CONFIG_64BIT))
-		seq_puts(m, "  device   can_id   can_mask      function          userdata       matches  ident\n");
-	else
-		seq_puts(m, "  device   can_id   can_mask  function  userdata   matches  ident\n");
+	seq_puts(m, "  device   can_id   can_mask   matches  sock_inode  function\n");
 }
 
 static int can_stats_proc_show(struct seq_file *m, void *v)
 {
 	struct net *net = m->private;
net/can/raw.c +2 -2
index 0acd4f6c6dd6..98808a75212e 100644
@@ -224,11 +224,11 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 	int i;
 
 	for (i = 0; i < count; i++) {
 		err = can_rx_register(net, dev, filter[i].can_id,
 				      filter[i].can_mask,
-				      raw_rcv, sk, "raw", sk);
+				      raw_rcv, sk, sock_i_ino(sk), sk);
 		if (err) {
 			/* clean up successfully registered filters */
 			while (--i >= 0)
 				can_rx_unregister(net, dev, filter[i].can_id,
 						  filter[i].can_mask,

@@ -245,11 +245,11 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
 {
 	int err = 0;
 
 	if (err_mask)
 		err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
-				      raw_rcv, sk, "raw", sk);
+				      raw_rcv, sk, sock_i_ino(sk), sk);
 
 	return err;
 }
 
 static void raw_disable_filters(struct net *net, struct net_device *dev,
Signature · 1 line
2.43.0
author maintainer
#6 lore raw

PATCH v3 3/4 can: replace ARPHRD_CAN type checks with can_get_ml_priv()

In reply to Oliver Hartkopp

Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device") introduced an explicit way to assign the midlayer private pointer (dev->ml_priv) to named users like ML_PRIV_CAN. A valid pointer returned by can_get_ml_priv() therefore identifies a real CAN device.

The referenced commit only converted the j1939 and proc code to this check, while the other parts of the CAN subsystem still rely on dev->type == ARPHRD_CAN.

The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed netdevice passes the ARPHRD_CAN checks and triggers a NULL pointer dereference when CAN filters are created and when they are removed, e.g. by the CAN protocols' netdevice notifier handlers. This only requires owning a TUN/TAP device or CAP_NET_ADMIN in an unprivileged user namespace.

Convert the remaining ARPHRD_CAN checks in af_can, bcm, gw, isotp and raw to can_get_ml_priv().

Fixes 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")
Cc [email protected]
Cc Oleksij Rempel <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
Link https://patch.msgid.link/[email protected] open here
Signed-off-by Marc Kleine-Budde <[email protected]>
---
 net/can/af_can.c | 15 ++++++---------
 net/can/bcm.c    |  7 ++++---
 net/can/gw.c     |  7 ++++---
 net/can/isotp.c  |  5 +++--
 net/can/raw.c    |  4 ++--
 5 files changed, 19 insertions(+), 19 deletions(-)
net/can/af_can.c +6 -9
index 6ebe9d3689ac..d97f85fc3232 100644
@@ -224,11 +224,11 @@ int can_send(struct sk_buff *skb, int loop)
 	if (unlikely(skb->len > READ_ONCE(skb->dev->mtu))) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
-	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+	if (unlikely(!can_get_ml_priv(skb->dev))) {
 		err = -EPERM;
 		goto inval_skb;
 	}
 
 	if (unlikely(!(skb->dev->flags & IFF_UP))) {

@@ -450,11 +450,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 
 	/* insert new receiver  (dev,canid,mask) -> (func,data) */
 
-	if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
+	if (dev && !can_get_ml_priv(dev))
 		return -ENODEV;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return -ENODEV;
 

@@ -517,11 +517,11 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *rcv = NULL;
 	struct hlist_head *rcv_list;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
-	if (dev && dev->type != ARPHRD_CAN)
+	if (dev && !can_get_ml_priv(dev))
 		return;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return;
 

@@ -682,12 +682,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_can_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_can_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CAN_RX_INVALID_FRAME);
 		return NET_RX_DROP;

@@ -708,12 +707,11 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canfd_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canfd_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANFD_RX_INVALID_FRAME);
 		return NET_RX_DROP;

@@ -734,12 +732,11 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canxl_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_canxl_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANXL_RX_INVALID_FRAME);
 		return NET_RX_DROP;
net/can/bcm.c +4 -3
index 6c85a76477c3..6759662e024c 100644
@@ -52,10 +52,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>

@@ -1716,11 +1717,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			if (!dev) {
 				ret = -ENODEV;
 				goto out_release;
 			}
 
-			if (dev->type != ARPHRD_CAN) {
+			if (!can_get_ml_priv(dev)) {
 				dev_put(dev);
 				ret = -ENODEV;
 				goto out_release;
 			}
 

@@ -1864,11 +1865,11 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(bcm_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;

@@ -2021,11 +2022,11 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 		dev = dev_get_by_index(net, addr->can_ifindex);
 		if (!dev) {
 			ret = -ENODEV;
 			goto fail;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			dev_put(dev);
 			ret = -ENODEV;
 			goto fail;
 		}
net/can/gw.c +4 -3
index 36f9aa0f7026..8a5327d8a00e 100644
@@ -50,10 +50,11 @@
 #include <linux/net.h>
 #include <linux/netdevice.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/gw.h>
 #include <net/can.h>
 #include <net/rtnetlink.h>

@@ -610,11 +611,11 @@ static int cgw_notifier(struct notifier_block *nb,
 			unsigned long msg, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net *net = dev_net(dev);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 
 	if (msg == NETDEV_UNREGISTER) {
 		struct cgw_job *gwj = NULL;
 		struct hlist_node *nx;

@@ -1161,19 +1162,19 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	gwj->src.dev = __dev_get_by_index(net, gwj->ccgw.src_idx);
 
 	if (!gwj->src.dev)
 		goto out;
 
-	if (gwj->src.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->src.dev))
 		goto out;
 
 	gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx);
 
 	if (!gwj->dst.dev)
 		goto out;
 
-	if (gwj->dst.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->dst.dev))
 		goto out;
 
 	/* is sending the skb back to the incoming interface intended? */
 	if (gwj->src.dev == gwj->dst.dev &&
 	    !(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK)) {
net/can/isotp.c +3 -2
index b239f4b45518..1a272ce6a2bd 100644
@@ -63,10 +63,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
 #include <linux/slab.h>
 #include <net/can.h>

@@ -1608,11 +1609,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	dev = dev_get_by_index(net, addr->can_ifindex);
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
 		err = -ENODEV;
 		goto out_put_dev;
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
 		err = -EINVAL;

@@ -1895,11 +1896,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 			  void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(isotp_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;
net/can/raw.c +2 -2
index 98808a75212e..09a5c0275ae6 100644
@@ -346,11 +346,11 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 static int raw_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(raw_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;

@@ -489,11 +489,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
 
 		if (!(dev->flags & IFF_UP))
Signature · 1 line
2.43.0
author maintainer
#7 lore raw

PATCH v3 4/4 can: protect the receive lists with a per-device lock

In reply to Oliver Hartkopp

The receive lists of a CAN device are located in the device's can_ml_priv and travel with the device when it is moved to another network namespace. But they are protected by the per-netns rcvlists_lock. So can_rx_unregister() needs dev_net(dev) to pick the right lock and silently returns when the passed namespace does not match dev_net(dev) anymore.

isotp_release() and bcm_release() leave their notifier list before they take any lock that serializes against the netdev notifier, which runs under RTNL. When the bound device is moved to another namespace in this window, the notifier skips the socket and the release path has to remove the filters itself. isotp_release() passes the socket's namespace, which does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev), which can_rx_unregister() reads again for its check, so a namespace change between both reads makes the check fail as well. In both cases can_rx_unregister() silently returns. As can_rx_register() stores rcv->sk without a reference, the remaining receivers point to the freed socket (isotp) or the freed op (bcm):

  BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0
  Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578
   can_rcv_filter+0x4af/0x8c0
   can_receive+0x28d/0x3c0
   can_rcv+0x2a9/0x310
   __netif_receive_skb_one_core+0x21a/0x260
   process_backlog+0x210/0x760

Besides this, the lists of a device are protected by the rcvlists_lock of the namespace that is passed in. Callers which pass dev_net(dev) read it at different times, so the same lists may be modified under the locks of different namespaces while the device is moved.

Taking RTNL in these release paths is no option: raw_bind(), raw_setsockopt() and raw_release() already hold RTNL while adding and removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged users can trigger repeatedly and which makes RTNL a global bottleneck.

Instead fix the root cause and add a spinlock to struct can_dev_rcv_lists, which protects the receive lists of a CAN device or the 'any' CAN device lists of a namespace. It is initialized in can_set_ml_priv(), which is used by all CAN network devices. With this, can_rx_unregister() finds the subscription independently of the current namespace of the device. The protocols keep the device valid with their tracked references. The per-netns rcvlists_lock only protects the receive list statistics anymore, and the callers pass the namespace that has been used for can_rx_register().

So isotp_release() and bcm_release() now remove their filters correctly without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and isotp_bind() do not need RTNL anymore and add and remove their filters under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind() publishes the device inside the RCU section of its lookup, and raw_notify() and isotp_notify() recheck the bound device under lock_sock().

Fixes 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
Fixes 8e8cda6d737d ("can: initial support for network namespaces")
Reported-by Norbert Szetei <[email protected]>
Link https://lore.kernel.org/linux-can/[email protected]/ open here
Cc [email protected]
Assisted-by Claude:claude-opus-5-5
Signed-off-by Oliver Hartkopp <[email protected]>

---
Changes in v3:
- not RFC anymore, sent as new series with the patches 1-3 from the linux-can-next pull request to build on top of mainline (see cover letter)
- commit message: describe the isotp and bcm cases separately (bcm passes dev_net(dev), which can_rx_unregister() reads again for its check) and mention that the lists of a device could be modified under the locks of different namespaces
- can_rx_unregister() kernel-doc: @net also selects the 'any' CAN device lists for @dev == NULL
- v2: https://lore.kernel.org/linux-can/[email protected]/

Changes in v2:
- v1 relied on the NETDEV_UNREGISTER rebroadcast in netdev_wait_allrefs_any() to catch a bind() racing with the notifier, which is not sent on a namespace change. raw_bind() and isotp_bind() now publish the device inside the RCU section of dev_get_by_index_rcu() (CAN_RAW: new ro->bind_dev hint with smp_store_release() / smp_load_acquire(), CAN_ISOTP: so->dev is published early and reset when bind() fails)
- moved the implementation details from the commit message to the notes
- shortened the code comments
- replaced the rebind rate measurement with an RTNL latency measurement
- v1: https://lore.kernel.org/linux-can/[email protected]/

Notes:

- bind() vs. NETDEV_UNREGISTER: Both on unregistration and on a namespace change, unlist_netdevice() and synchronize_net() precede NETDEV_UNREGISTER. bind() publishes the device (ro->bind_dev / so->dev) inside the RCU section of dev_get_by_index_rcu(). So either the notifier sees it after synchronize_net() and handles the socket under lock_sock(), or the lookup does not find the unlisted device and bind() fails with -ENODEV. The NETDEV_UNREGISTER rebroadcast in netdev_wait_allrefs_any() is not relied on, as it does not happen on a namespace change.

- raw_notify() / isotp_notify() keep a lockless READ_ONCE() fast path, so RTNL only waits for lock_sock() of sockets that are bound or being bound to the vanishing device. CAN_RAW needs the separate ro->bind_dev hint, as ro->dev still points to the previously bound device until a rebind completes. ro->bind_dev is never dereferenced; a stale value only causes a needless lock_sock(). raw_notify() reads ro->bind_dev with smp_load_acquire() before ro->dev, paired with smp_store_release() in raw_bind(): A later bind() can only overwrite ro->bind_dev after the previous bind() has set ro->dev and released lock_sock(). Otherwise the notifier could see a stale ro->dev and the ro->bind_dev of a subsequent failing rebind to another device, and miss the binding. CAN_ISOTP does not support rebinding and publishes so->dev directly, which is reset when bind() fails. bcm_notify() always takes lock_sock() and bcm_rx_setup() looks up the device under lock_sock(), so bcm needs no extra handling.

- raw_setsockopt() copies the filters from user space before lock_sock(), so a stalled copy can not block a NETDEV_UNREGISTER that waits for lock_sock() under RTNL. The dev->reg_state check is only a shortcut now (READ_ONCE()) and does not catch a namespace change.

- The NETDEV_DOWN case keeps checking ro->dev / so->dev locklessly. They are written with WRITE_ONCE() in bind() and in the notifier.

- j1939 benefits as well: After a namespace change its can_rx_unregister() with dev_net(ndev) picked the rcvlists_lock of the wrong namespace. The receive list statistics may still become imprecise in this case, as before.

- Tested on a lockdep + KASAN kernel:
- 10 min CAN_RAW / CAN_BCM / CAN_ISOTP traffic and procfs reads in two namespaces while moving the CAN device between them (2272 moves)
- CAN_RAW / CAN_ISOTP bind and CAN_RAW rebind churn (incl. failing rebinds to a non-CAN device) against 1007 namespace moves, checking that no filters are left on the device in the other namespace, and against 611 device deletions
- bcm_release() with a stalled procfs reader does not block RTNL
- RTNL latency (no-op SIOCSIFMTU on lo) under CAN_RAW 512 filter

    churn: avg ~1 us, max ~1.2 ms (before: avg ~250 us, max ~24 ms)
 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  38 ++++++----
 net/can/bcm.c              |  43 ++++-------
 net/can/isotp.c            |  40 +++++++----
 net/can/raw.c              | 142 ++++++++++++++++++++++++-------------
 6 files changed, 160 insertions(+), 108 deletions(-)
include/linux/can/can-ml.h +3 -0
index 1e99fda2b380..cfefa4a385be 100644
@@ -43,10 +43,11 @@
 #define CAN_ML_H
 
 #include <linux/can.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/spinlock.h>
 
 /* exposed CAN device capabilities for network layer */
 #define CAN_CAP_CC BIT(0) /* CAN CC aka Classical CAN */
 #define CAN_CAP_FD BIT(1) /* CAN FD */
 #define CAN_CAP_XL BIT(2) /* CAN XL */

@@ -57,10 +58,11 @@
 #define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)
 
 enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX };
 
 struct can_dev_rcv_lists {
+	spinlock_t lock; /* protects the receive lists and entries */
 	struct hlist_head rx[RX_MAX];
 	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
 	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
 	int entries;
 };

@@ -79,10 +81,11 @@ static inline struct can_ml_priv *can_get_ml_priv(struct net_device *dev)
 }
 
 static inline void can_set_ml_priv(struct net_device *dev,
 				   struct can_ml_priv *ml_priv)
 {
+	spin_lock_init(&ml_priv->dev_rcv_lists.lock);
 	netdev_set_ml_priv(dev, ml_priv, ML_PRIV_CAN);
 }
 
 static inline bool can_cap_enabled(struct net_device *dev, u32 cap)
 {
include/net/netns/can.h +1 -1
index 48b79f7e6236..ef7aefdf7b23 100644
@@ -27,11 +27,11 @@ struct netns_can {
 	struct proc_dir_entry *bcmproc_dir;
 #endif
 
 	/* receive filters subscribed for 'all' CAN devices */
 	struct can_dev_rcv_lists *rx_alldev_list;
-	spinlock_t rcvlists_lock;
+	spinlock_t rcvlists_lock; /* protects rcv_lists_stats */
 	struct timer_list stattimer; /* timer for statistics update */
 	struct can_pkg_stats *pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats;
 
 	/* CAN GW per-net gateway jobs */
net/can/af_can.c +24 -14
index d97f85fc3232..bf612c4b983d 100644
@@ -460,13 +460,14 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
 	if (!rcv)
 		return -ENOMEM;
 
-	spin_lock_bh(&net->can.rcvlists_lock);
-
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+
+	spin_lock_bh(&dev_rcv_lists->lock);
+
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	rcv->can_id = can_id;
 	rcv->mask = mask;
 	atomic_long_set(&rcv->matches, 0);

@@ -476,10 +477,13 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	rcv->sk = sk;
 
 	hlist_add_head_rcu(&rcv->list, rcv_list);
 	dev_rcv_lists->entries++;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	rcv_lists_stats->rcv_entries++;
 	rcv_lists_stats->rcv_entries_max = max(rcv_lists_stats->rcv_entries_max,
 					       rcv_lists_stats->rcv_entries);
 	spin_unlock_bh(&net->can.rcvlists_lock);
 

@@ -507,10 +511,17 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  * @func: callback function on filter match
  * @data: returned parameter for callback function
  *
  * Description:
  *  Removes subscription entry depending on given (subscription) values.
+ *  The receive lists of a CAN device are protected by a per-device lock,
+ *  so the subscription is found even when the device has been moved to
+ *  another network namespace after can_rx_register(). The caller has to
+ *  ensure that @dev stays valid, e.g. by holding a reference to @dev or
+ *  RTNL. @net is the namespace that has been used for can_rx_register().
+ *  It selects the 'any' CAN device lists for @dev == NULL and is used for
+ *  the receive list statistics.
  */
 void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
 		       void *data)
 {

@@ -520,16 +531,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 
-	spin_lock_bh(&net->can.rcvlists_lock);
+	spin_lock_bh(&dev_rcv_lists->lock);
 
-	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/* Search the receiver list for the item to delete.  This should
 	 * exist, since no receiver may be unregistered that hasn't
 	 * been registered before.

@@ -545,30 +554,30 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * As this case may potentially happen when closing a socket while
 	 * the notifier for removing the CAN netdev is running we just print
 	 * a warning here.
 	 */
 	if (!rcv) {
+		spin_unlock_bh(&dev_rcv_lists->lock);
 		pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
 			DNAME(dev), can_id, mask);
-		goto out;
+		return;
 	}
 
 	hlist_del_rcu(&rcv->list);
 	dev_rcv_lists->entries--;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	if (rcv_lists_stats->rcv_entries > 0)
 		rcv_lists_stats->rcv_entries--;
-
- out:
 	spin_unlock_bh(&net->can.rcvlists_lock);
 
 	/* schedule the receiver item for deletion */
-	if (rcv) {
-		if (rcv->sk)
-			sock_hold(rcv->sk);
-		call_rcu(&rcv->rcu, can_rx_delete_receiver);
-	}
+	if (rcv->sk)
+		sock_hold(rcv->sk);
+	call_rcu(&rcv->rcu, can_rx_delete_receiver);
 }
 EXPORT_SYMBOL(can_rx_unregister);
 
 static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
 {

@@ -821,10 +830,11 @@ static int can_pernet_init(struct net *net)
 {
 	spin_lock_init(&net->can.rcvlists_lock);
 	net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list);
 	if (!net->can.rx_alldev_list)
 		goto out;
+	spin_lock_init(&net->can.rx_alldev_list->lock);
 	net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats);
 	if (!net->can.pkg_stats)
 		goto out_free_rx_alldev_list;
 	net->can.rcv_lists_stats = kzalloc_obj(*net->can.rcv_lists_stats);
 	if (!net->can.rcv_lists_stats)
net/can/bcm.c +12 -31

91 lines not on this page · raw message

net/can/isotp.c +28 -12
index 1a272ce6a2bd..c91b4d2e9c92 100644
@@ -1487,15 +1487,15 @@ static int isotp_release(struct socket *sock)
 		spin_lock(&isotp_notifier_lock);
 	}
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
 	/* remove current filters & unregister
-	 * tracked reference so->dev is taken at bind() time with rtnl_lock
+	 * the tracked reference so->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
 			can_rx_unregister(net, so->dev, so->rxid,
 					  SINGLE_MASK(so->rxid),

@@ -1509,12 +1509,10 @@ static int isotp_release(struct socket *sock)
 
 	so->ifindex = 0;
 	so->bound = 0;
 	so->dev = NULL;
 
-	rtnl_unlock();
-
 	/* Always wait for a grace period before touching the timers below.
 	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
 	 * filters and cleared so->bound in isotp_notify() without waiting
 	 * for in-flight isotp_rcv() callers to finish, so this call must not
 	 * be skipped just because so->bound is already 0 here.

@@ -1578,11 +1576,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 
 	if (!addr->can_ifindex)
 		return -ENODEV;
 
-	rtnl_lock();
+	/* lock_sock() serializes the binding with isotp_notify() */
 	lock_sock(sk);
 
 	if (so->bound) {
 		err = -EINVAL;
 		goto out;

@@ -1604,11 +1602,21 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	if (isotp_register_rxid(so) && rx_id == tx_id) {
 		err = -EADDRNOTAVAIL;
 		goto out;
 	}
 
-	dev = dev_get_by_index(net, addr->can_ifindex);
+	/* Publish the device inside the RCU section of the lookup: When it
+	 * is unlisted concurrently, either isotp_notify() sees so->dev after
+	 * synchronize_net() or the lookup fails.
+	 */
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
+	if (dev) {
+		dev_hold(dev);
+		WRITE_ONCE(so->dev, dev);
+	}
+	rcu_read_unlock();
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
 	if (!can_get_ml_priv(dev)) {

@@ -1617,11 +1625,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
 		err = -EINVAL;
 		goto out_put_dev;
 	}
-	if (!(dev->flags & IFF_UP))
+	if (!(READ_ONCE(dev->flags) & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
 
 	if (isotp_register_rxid(so))

@@ -1643,19 +1651,21 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	/* bind() ok -> hold a reference for so->dev so that isotp_release()
 	 * can safely reach the device later, even if a concurrent
 	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
 	 */
-	so->dev = dev;
 	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* the published so->dev is only valid for a successful bind() */
+	if (err)
+		WRITE_ONCE(so->dev, NULL);
+
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);

@@ -1854,16 +1864,22 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	struct sock *sk = &so->sk;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->dev != dev)
+	/* lockless fast path, see raw_notify() */
+	if (READ_ONCE(so->dev) != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (so->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (so->bound) {
 			if (isotp_register_rxid(so))
 				can_rx_unregister(dev_net(dev), dev, so->rxid,
 						  SINGLE_MASK(so->rxid),

@@ -1875,11 +1891,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 			netdev_put(so->dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
-		so->dev = NULL;
+		WRITE_ONCE(so->dev, NULL);
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
net/can/raw.c +92 -50

361 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#8 lore raw
In reply to Oliver Hartkopp
Quoted 9 lines · Oliver Hartkopp

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next with the linux-can-next pull request [2]. They are included unchanged only to provide a sequence that applies on top of mainline, so that the sashiko bot is able to build and review patch 4. Please do not apply patches 1-3 from this series.

This doesn't work for me.

Please post a single v5 with all of your patches based on linux-can-next/testing.

regards,
Marc

Signature · 4 lines
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   |
author maintainer
#9 lore raw
Quoted 6 lines · Marc Kleine-Budde
Quoted 5 lines · Oliver Hartkopp

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next

As these three patches "1-3" are not in your current tree anymore you can simply apply this complete series as-is.

And on top if this series you can also apply this

https://lore.kernel.org/linux-can/[email protected]/

to fix this sashiko-bot remark:

https://lore.kernel.org/linux-can/[email protected]/

Best regards,
Oliver

Quoted 13 lines
Quoted 4 lines

with the linux-can-next pull request [2]. They are included unchanged only to provide a sequence that applies on top of mainline, so that the sashiko bot is able to build and review patch 4. Please do not apply patches 1-3 from this series.

This doesn't work for me.

Please post a single v5 with all of your patches based on linux-can-next/testing.

regards,
Marc

maintainer
#10 lore raw
Quoted 13 lines · Oliver Hartkopp
Quoted 6 lines · Marc Kleine-Budde
Quoted 5 lines · Oliver Hartkopp

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next

As these three patches "1-3" are not in your current tree anymore you can simply apply this complete series as-is.

Quoted 7 lines

And on top if this series you can also apply this

https://lore.kernel.org/linux-can/[email protected]/

to fix this sashiko-bot remark:

https://lore.kernel.org/linux-can/[email protected]/

It doesn't work for me. Please send a _single_ series on top of the testing branch, which includes _all_ patches.

regards,
Marc

| $ b4 shazam -s -k -l -H --merge-base=@ https://lore.kernel.org/linux-can/[email protected]/
| Looking up [email protected]
| Checking for newer revisions
| Grabbing search results from lore.kernel.org
| Analyzing 1 messages in the thread
| Looking for additional code-review trailers on lore.kernel.org
| Analyzing 0 code-review messages
| Checking attestation on all messages, may take a moment...
| Retrieving patchwork CI status, may take a moment...
| ---
|   ✓ [PATCH] can: proc: skip stats calculation after reset in can_stat_update()
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ---
|   ✓ Signed: DKIM/hartkopp.net
| ---
| Total patches: 1
| ---
|  Base: attempting to guess base-commit...
|  Base: remotes/net-next/HEAD-5-gfdb32e6b9bdc (exact match)
|  Base: @
| Magic: Preparing a sparse worktree
| Unable to cleanly apply series, see failure log below
| ---
| Applying: can: proc: skip stats calculation after reset in can_stat_update()
| Using index info to reconstruct a base tree...
| M       net/can/proc.c
| Falling back to patching base and 3-way merge...
| Auto-merging net/can/proc.c
| CONFLICT (content): Merge conflict in net/can/proc.c
| Patch failed at 0001 can: proc: skip stats calculation after reset in can_stat_update()
| Recorded preimage for 'net/can/proc.c'
| error: Failed to merge in the changes.
| hint: Use 'git am --show-current-patch=diff' to see the failed patch
| ---
| Not fetching into FETCH_HEAD
| Use --resolve to enable conflict resolution
Signature · 4 lines
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   |
maintainer
#11 lore raw
Quoted 22 lines · Marc Kleine-Budde
Quoted 13 lines · Oliver Hartkopp
Quoted 6 lines · Marc Kleine-Budde
Quoted 5 lines · Oliver Hartkopp

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next

As these three patches "1-3" are not in your current tree anymore you can simply apply this complete series as-is.

Quoted 7 lines

And on top if this series you can also apply this

https://lore.kernel.org/linux-can/[email protected]/

to fix this sashiko-bot remark:

https://lore.kernel.org/linux-can/[email protected]/

Quoted 7 lines

It doesn't work for me. Please send a _single_ series on top of the testing branch, which includes _all_ patches.

regards,
Marc

| $ b4 shazam -s -k -l -H --merge-base=@ https://lore.kernel.org/linux-can/[email protected]/

Ok, that was the wrong series, but [email protected] doesn't work either.

Marc

| $ b4 shazam -s -k -l -H --merge-base=@ [email protected]
| Looking up [email protected]
| Checking for newer revisions
| Grabbing search results from lore.kernel.org
| Analyzing 11 messages in the thread
| Looking for additional code-review trailers on lore.kernel.org
| Analyzing 19 code-review messages
| Checking attestation on all messages, may take a moment...
| Retrieving patchwork CI status, may take a moment...
| ---
|   ✓ [PATCH v3 1/4] can: proc: reset pkg_stats atomics individually
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 2/4] can: proc: remove pointers from CAN specific proc output
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 3/4] can: replace ARPHRD_CAN type checks with can_get_ml_priv()
|     + Link: https://patch.msgid.link/[email protected]
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 4/4] can: protect the receive lists with a per-device lock
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ---
|   ✓ Signed: DKIM/hartkopp.net
| ---
| Total patches: 4
| ---
|  Base: @
| Magic: Preparing a sparse worktree
| Unable to cleanly apply series, see failure log below
| ---
| Applying: can: proc: reset pkg_stats atomics individually
| Applying: can: proc: remove pointers from CAN specific proc output
| Applying: can: replace ARPHRD_CAN type checks with can_get_ml_priv()
| Applying: can: protect the receive lists with a per-device lock
| Using index info to reconstruct a base tree...
| M       net/can/isotp.c
| M       net/can/raw.c
| Falling back to patching base and 3-way merge...
| Auto-merging net/can/isotp.c
| Auto-merging net/can/raw.c
| CONFLICT (content): Merge conflict in net/can/raw.c
| Patch failed at 0004 can: protect the receive lists with a per-device lock
| Resolved 'net/can/raw.c' using previous resolution.
| error: Failed to merge in the changes.
| hint: Use 'git am --show-current-patch=diff' to see the failed patch
| ---
| Not fetching into FETCH_HEAD
| Use --resolve to enable conflict resolution
Signature · 4 lines
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   |
author maintainer
#12 lore raw

can: raw: remove redundant NULL check before netdev_hold()

was missing in my tree.

v4 is sent

Many thanks,
Oliver

Quoted 89 lines · Marc Kleine-Budde
Quoted 22 lines · Marc Kleine-Budde
Quoted 13 lines · Oliver Hartkopp
Quoted 6 lines · Marc Kleine-Budde
Quoted 5 lines · Oliver Hartkopp

This series fixes a use-after-free in CAN_ISOTP and CAN_BCM that can be triggered when a CAN device is moved to another network namespace while a socket bound to this device is released [1].

Only patch 4 is new. Patches 1-3 are already on their way to net-next

As these three patches "1-3" are not in your current tree anymore you can simply apply this complete series as-is.

Quoted 7 lines

And on top if this series you can also apply this

https://lore.kernel.org/linux-can/[email protected]/

to fix this sashiko-bot remark:

https://lore.kernel.org/linux-can/[email protected]/

Quoted 7 lines

It doesn't work for me. Please send a _single_ series on top of the testing branch, which includes _all_ patches.

regards,
Marc

| $ b4 shazam -s -k -l -H --merge-base=@ https://lore.kernel.org/linux-can/[email protected]/

Ok, that was the wrong series, but [email protected] doesn't work either.

Marc

| $ b4 shazam -s -k -l -H --merge-base=@ [email protected]
| Looking up [email protected]
| Checking for newer revisions
| Grabbing search results from lore.kernel.org
| Analyzing 11 messages in the thread
| Looking for additional code-review trailers on lore.kernel.org
| Analyzing 19 code-review messages
| Checking attestation on all messages, may take a moment...
| Retrieving patchwork CI status, may take a moment...
| ---
|   ✓ [PATCH v3 1/4] can: proc: reset pkg_stats atomics individually
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 2/4] can: proc: remove pointers from CAN specific proc output
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 3/4] can: replace ARPHRD_CAN type checks with can_get_ml_priv()
|     + Link: https://patch.msgid.link/[email protected]
|     ● checkpatch.pl: passed all checks
|   ✓ [PATCH v3 4/4] can: protect the receive lists with a per-device lock
|     + Link: https://patch.msgid.link/[email protected]
|     + Signed-off-by: Marc Kleine-Budde <[email protected]>
|     ● checkpatch.pl: passed all checks
|   ---
|   ✓ Signed: DKIM/hartkopp.net
| ---
| Total patches: 4
| ---
|  Base: @
| Magic: Preparing a sparse worktree
| Unable to cleanly apply series, see failure log below
| ---
| Applying: can: proc: reset pkg_stats atomics individually
| Applying: can: proc: remove pointers from CAN specific proc output
| Applying: can: replace ARPHRD_CAN type checks with can_get_ml_priv()
| Applying: can: protect the receive lists with a per-device lock
| Using index info to reconstruct a base tree...
| M       net/can/isotp.c
| M       net/can/raw.c
| Falling back to patching base and 3-way merge...
| Auto-merging net/can/isotp.c
| Auto-merging net/can/raw.c
| CONFLICT (content): Merge conflict in net/can/raw.c
| Patch failed at 0004 can: protect the receive lists with a per-device lock
| Resolved 'net/can/raw.c' using previous resolution.
| error: Failed to merge in the changes.
| hint: Use 'git am --show-current-patch=diff' to see the failed patch
| ---
| Not fetching into FETCH_HEAD
| Use --resolve to enable conflict resolution