* [PATCH v8 1/7] can: proc: reset pkg_stats atomics individually
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
@ 2026-10-23 12:17 ` Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 2/7] can: proc: skip stats calculation after reset in can_stat_update() Oliver Hartkopp
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:17 UTC (permalink / raw)
To: linux-can; +Cc: Runyu Xiao, stable, Oliver Hartkopp
From: Runyu Xiao <runyu.xiao@seu.edu.cn>
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: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/proc.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/net/can/proc.c b/net/can/proc.c
index de4d05ae3459..70104f13077e 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -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));
}
/*
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 2/7] can: proc: skip stats calculation after reset in can_stat_update()
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 1/7] can: proc: reset pkg_stats atomics individually Oliver Hartkopp
@ 2026-10-23 12:17 ` Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 3/7] can: proc: remove pointers from CAN specific proc output Oliver Hartkopp
` (6 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:17 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path")
introduced local snapshots of the statistics counters at the beginning
of can_stat_update(). When can_init_stats() resets the statistics, these
stale pre-reset snapshots are still used for the following total/current
/max calculations and overwrite the freshly reset values. Additionally a
jiffies tick between the snapshot and the reset could trigger a second
can_init_stats() call.
Merge the reset conditions into a single check and skip the calculation
for the current interval when the statistics have been reset.
Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/proc.c | 30 ++++++++++++------------------
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/net/can/proc.c b/net/can/proc.c
index 70104f13077e..fb144d2adc35 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -147,29 +147,22 @@ void can_stat_update(struct timer_list *t)
/* 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);
-
- /* prevent overflow in calc_rate() */
- if (tx_frames > (LONG_MAX / HZ))
- can_init_stats(net);
-
- /* matches overflow - very improbable */
- if (matches > (LONG_MAX / 100))
+ /*
+ * Restart counting in timer context on user request, on jiffies
+ * overflow or to prevent overflows in calc_rate() and in the
+ * match ratio calculation. The local snapshots are stale after
+ * the reset, so skip the calculation for this interval.
+ */
+ if (user_reset || j < pkg_stats->jiffies_init ||
+ rx_frames > (LONG_MAX / HZ) || tx_frames > (LONG_MAX / HZ) ||
+ matches > (LONG_MAX / 100)) {
can_init_stats(net);
+ goto out;
+ }
/* calc total values */
if (rx_frames)
pkg_stats->total_rx_match_ratio = (matches * 100) / rx_frames;
@@ -194,10 +187,11 @@ 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;
+out:
/* restart timer (one second) */
mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ));
}
/*
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 3/7] can: proc: remove pointers from CAN specific proc output
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 1/7] can: proc: reset pkg_stats atomics individually Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 2/7] can: proc: skip stats calculation after reset in can_stat_update() Oliver Hartkopp
@ 2026-10-23 12:17 ` Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 4/7] can: replace ARPHRD_CAN type checks with can_get_ml_priv() Oliver Hartkopp
` (5 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:17 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Sebastian Andrzej Siewior
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 <bigeasy@linutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
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(-)
diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
index 536ff411da1d..51bc70748f75 100644
--- a/Documentation/networking/can.rst
+++ b/Documentation/networking/can.rst
@@ -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
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 2de74c2b78b6..effb5c31ef40 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -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);
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 34fe3b28d576..6ebe9d3689ac 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -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++;
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 87887014f562..6e593ed5db5f 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -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 */
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..6c85a76477c3 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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;
diff --git a/net/can/gw.c b/net/can/gw.c
index 5793cb2420ed..36f9aa0f7026 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -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,
diff --git a/net/can/isotp.c b/net/can/isotp.c
index f5dc9d04bd68..b239f4b45518 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -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;
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..d9384cf0e356 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -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;
}
diff --git a/net/can/proc.c b/net/can/proc.c
index fb144d2adc35..b7496d2a37cd 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -202,30 +202,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;
diff --git a/net/can/raw.c b/net/can/raw.c
index 6944ebab3feb..3e57f72a3afe 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -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,
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 4/7] can: replace ARPHRD_CAN type checks with can_get_ml_priv()
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (2 preceding siblings ...)
2026-10-23 12:17 ` [PATCH v8 3/7] can: proc: remove pointers from CAN specific proc output Oliver Hartkopp
@ 2026-10-23 12:17 ` Oliver Hartkopp
2026-10-23 12:17 ` [PATCH v8 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind() Oliver Hartkopp
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:17 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, stable, Oleksij Rempel, Marc Kleine-Budde
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.
Commit 8aa59e355949 ("can: af_can: fix NULL pointer dereference in
can_rx_register()") already made can_rx_register() reject such devices.
But can_rx_unregister() still only checks dev->type and dereferences
the NULL ml_priv when it walks the receive lists of the device.
CAN_ISOTP reaches this path: isotp_bind() ignores the return values of
can_rx_register() and binds the socket anyway. When the socket is
released or the device is unregistered, isotp_release() and
isotp_notify() remove the filters that have never been registered and
trigger the NULL pointer dereference in can_rx_unregister(). 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: stable@kernel.org
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
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(-)
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 6ebe9d3689ac..d97f85fc3232 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -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;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6c85a76477c3..6759662e024c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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;
}
diff --git a/net/can/gw.c b/net/can/gw.c
index 36f9aa0f7026..8a5327d8a00e 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -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)) {
diff --git a/net/can/isotp.c b/net/can/isotp.c
index b239f4b45518..1a272ce6a2bd 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -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;
diff --git a/net/can/raw.c b/net/can/raw.c
index 3e57f72a3afe..dfea48768b23 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -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))
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind()
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (3 preceding siblings ...)
2026-10-23 12:17 ` [PATCH v8 4/7] can: replace ARPHRD_CAN type checks with can_get_ml_priv() Oliver Hartkopp
@ 2026-10-23 12:17 ` Oliver Hartkopp
2026-10-23 12:18 ` [PATCH v8 6/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:17 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, stable
isotp_bind() ignores the return values of can_rx_register() for the RX
and the echo receiver and binds the socket anyway. When a receiver can
not be allocated, bind() succeeds without this receiver, so the socket
does not receive frames or its transmissions time out. isotp_release()
and isotp_notify() later try to remove the missing receiver, which
triggers the "receive list entry not found" warning in
can_rx_unregister().
As the device has been checked before and RTNL prevents a namespace
change during bind(), only -ENOMEM can be returned here.
Check the return values, remove an already registered RX receiver when
the echo receiver can not be registered and return the error.
Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/isotp.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 1a272ce6a2bd..da80ef606ce0 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1622,20 +1622,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
if (!(dev->flags & IFF_UP))
notify_enetdown = 1;
ifindex = dev->ifindex;
- if (isotp_register_rxid(so))
- can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
- isotp_rcv, sk, sock_i_ino(sk), sk);
+ if (isotp_register_rxid(so)) {
+ err = can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
+ isotp_rcv, sk, sock_i_ino(sk), sk);
+ if (err)
+ goto out_put_dev;
+ }
/* 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, sock_i_ino(sk), sk);
+ err = can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
+ isotp_rcv_echo, sk, sock_i_ino(sk), sk);
+ if (err) {
+ if (isotp_register_rxid(so))
+ can_rx_unregister(net, dev, rx_id, SINGLE_MASK(rx_id),
+ isotp_rcv, sk);
+ goto out_put_dev;
+ }
/* switch to new settings */
so->ifindex = ifindex;
so->rxid = rx_id;
so->txid = tx_id;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 6/7] can: protect the receive lists with a per-device lock
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (4 preceding siblings ...)
2026-10-23 12:17 ` [PATCH v8 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind() Oliver Hartkopp
@ 2026-10-23 12:18 ` Oliver Hartkopp
2026-10-23 12:18 ` [PATCH v8 7/7] can: raw, isotp: copy socket options outside of lock_sock() Oliver Hartkopp
` (2 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:18 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Norbert Szetei, stable
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 CAN_RAW, CAN_BCM and CAN_ISOTP
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().
As RTNL does not keep NETDEV_DOWN out of bind() anymore, bind() checks
IFF_UP after publishing the bound device, with a barrier pair against
raw_notify() and isotp_notify(), so a down transition during bind() is
still reported with ENETDOWN.
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 <norbert@doyensec.com>
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v8:
- rebased on the new patch "can: isotp: check the return values of
can_rx_register() in isotp_bind()": isotp_bind() resets the published
so->dev again when the registration of a receiver fails
- isotp_bind(): check IFF_UP at the end of a successful bind(), as
raw_bind() does, so that a failing bind() does not report ENETDOWN
- isotp_notify(): report NETDEV_DOWN only for a bound socket. As
isotp_bind() sets so->bound before its IFF_UP check, a failing
isotp_bind() can not get a needless ENETDOWN from the notifier anymore
- isotp_sendmsg() / isotp_recvmsg(): read so->bound with READ_ONCE(), as
it is written with WRITE_ONCE() now
- raw_setsockopt(): moved copying the user space data outside of
lock_sock() to a separate patch to keep this patch focused on the
locking changes and to ease review
- v7: https://lore.kernel.org/linux-can/20261022114202.180023-6-socketcan@hartkopp.net/
Changes in v7:
- raw_bind() / isotp_bind(): report ENETDOWN before release_sock(), so
that a concurrent NETDEV_UNREGISTER waiting for lock_sock() in
raw_notify() / isotp_notify() can not have its ENODEV overwritten by
ENETDOWN (Sashiko review)
- raw_notify() / isotp_notify(): report ENODEV for NETDEV_UNREGISTER
before release_sock() for consistency (Sashiko review)
- v6: https://lore.kernel.org/linux-can/20261021231130.2085690-12-mkl@pengutronix.de/
Changes in v6:
- isotp_bind(): check the device (CAN device, MTU) before publishing
so->dev, so bind() can not fail after so->dev has become visible to
isotp_notify(). A failing bind() can not get a needless ENETDOWN from
a concurrent NETDEV_DOWN anymore (Sashiko review)
- v5: https://lore.kernel.org/linux-can/20261020203257.230955-6-socketcan@hartkopp.net/
Changes in v5:
- raw_bind() / isotp_bind(): check IFF_UP after publishing the bound
device, with an smp_mb() pair against raw_notify() / isotp_notify() for
NETDEV_DOWN, so a down transition during bind() is reported with
ENETDOWN again, as with RTNL before (Sashiko review). isotp_bind()
already published so->dev before the IFF_UP check, but without a
barrier the load could still pass the store.
- raw_bind() no longer reports ENETDOWN when bind() fails
- rcvlists_lock comment names the rcv_entries(_max) stats (Sashiko review)
- can_rx_unregister() kernel-doc and commit message: @net should be the
namespace of can_rx_register(), otherwise only the statistics become
imprecise (j1939, Sashiko review)
- v4: https://lore.kernel.org/linux-can/20261019152741.54854-6-socketcan@hartkopp.net/
Changes in v4:
- rebased on linux-can-next/testing: the context in raw_bind() changed
with "can: raw: remove redundant NULL check before netdev_hold()",
which made v3 fail to apply
- added "can: proc: skip stats calculation after reset in
can_stat_update()" from linux-can-next/testing to the base sequence
- no functional change
- v3: https://lore.kernel.org/linux-can/20261019074244.50391-5-socketcan@hartkopp.net/
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/20261018180739.689750-1-socketcan@hartkopp.net/
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/20261017201935.399233-1-socketcan@hartkopp.net/
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 after checking the device and resets it when the registration
of a receiver 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(): The dev->reg_state check is only a shortcut now
(READ_ONCE()) and does not catch a namespace change. raw_notify()
waits for lock_sock() and removes the filters set by raw_setsockopt().
- NETDEV_DOWN: raw_notify() / isotp_notify() check ro->dev / so->dev
locklessly. They are written with WRITE_ONCE() in bind() and in the
notifier. IFF_UP is cleared in __dev_close_many() before
netif_close_many() or __dev_notify_flags() send NETDEV_DOWN in the
same context, and bind() checks IFF_UP after publishing the bound
device. With an smp_mb() on both sides, either the notifier sees the
binding or bind() sees the cleared IFF_UP. isotp_notify() additionally
requires so->bound, which isotp_bind() sets before its IFF_UP check, so
a failing isotp_bind() is not reported.
- 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 v8 on a lockdep + KASAN + failslab kernel and on a kernel
without debug options (numbers from the debug kernel):
- 10 min CAN_RAW / CAN_BCM / CAN_ISOTP traffic and procfs reads in two
namespaces while moving the CAN device between them (2220 moves)
- CAN_RAW / CAN_ISOTP bind and CAN_RAW rebind churn (incl. failing
rebinds to a non-CAN device) against 1001 namespace moves, checking
that no filters are left on the device in the other namespace, and
against 609 device deletions
- bind() vs. NETDEV_DOWN: bind() while the device is toggled down/up,
no missed ENETDOWN in 23024 CAN_RAW and 3906 CAN_ISOTP checks
- the same bind churn with failslab on the can_receiver cache (-ENOMEM
in can_rx_register()): no leaks, no "receive list entry not found"
- socket option set/get/error checks for CAN_RAW and CAN_ISOTP
- 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 ~0.1 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 | 39 +++++++++++------
net/can/bcm.c | 43 ++++++-------------
net/can/isotp.c | 84 ++++++++++++++++++++++--------------
net/can/raw.c | 88 ++++++++++++++++++++++++++++----------
6 files changed, 159 insertions(+), 100 deletions(-)
diff --git a/include/linux/can/can-ml.h b/include/linux/can/can-ml.h
index 1e99fda2b380..cfefa4a385be 100644
--- a/include/linux/can/can-ml.h
+++ b/include/linux/can/can-ml.h
@@ -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)
{
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 48b79f7e6236..54f23681e0d4 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -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 the rcv_entries(_max) 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 */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d97f85fc3232..0f3051a61aab 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -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,18 @@ 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 should be the namespace that has been used for
+ * can_rx_register(). It selects the 'any' CAN device lists for
+ * @dev == NULL and is otherwise only used for the receive list
+ * statistics, which become imprecise when another namespace is passed.
*/
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 +532,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 +555,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 +831,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)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6759662e024c..7bd2aa39777a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -960,11 +960,11 @@ static void bcm_remove_op(struct bcm_op *op)
}
static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
{
if (op->rx_reg_dev == dev) {
- can_rx_unregister(dev_net(dev), dev, op->can_id,
+ can_rx_unregister(sock_net(op->sk), dev, op->can_id,
REGMASK(op->can_id), bcm_rx_handler, op);
/* mark as removed subscription */
op->rx_reg_dev = NULL;
netdev_put(dev, &op->rx_reg_dev_tracker);
@@ -986,25 +986,14 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
(op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
/* disable automatic timer on frame reception */
op->flags |= RX_NO_AUTOTIMER;
- /*
- * Don't care if we're bound or not (due to netdev
- * problems) can_rx_unregister() is always a save
- * thing to do here.
- */
if (op->ifindex) {
- /*
- * Only remove subscriptions that had not
- * been removed due to NETDEV_UNREGISTER
- * in bcm_notifier()
- *
- * op->rx_reg_dev is a tracked reference taken
- * when the subscription was registered, so it
- * stays valid here even if a concurrent
- * NETDEV_UNREGISTER already unlisted the dev.
+ /* Only remove subscriptions that had not been
+ * removed in bcm_notify(). op->rx_reg_dev keeps
+ * the device's receive lists accessible.
*/
if (op->rx_reg_dev)
bcm_rx_unreg(op->rx_reg_dev, op);
} else
can_rx_unregister(sock_net(op->sk), NULL,
@@ -1933,10 +1922,11 @@ static int bcm_release(struct socket *sock)
spin_lock(&bcm_notifier_lock);
}
list_del(&bo->notifier);
spin_unlock(&bcm_notifier_lock);
+ /* bcm_notify() can not access this socket anymore */
lock_sock(sk);
#if IS_ENABLED(CONFIG_PROC_FS)
/* remove procfs entry */
if (net->can.bcmproc_dir && bo->bcm_proc_read)
@@ -1946,33 +1936,24 @@ static int bcm_release(struct socket *sock)
list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
list_del_rcu(&op->list);
bcm_remove_op(op);
}
- list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
- /*
- * Don't care if we're bound or not (due to netdev problems)
- * can_rx_unregister() is always a save thing to do here.
- */
+ list_for_each_entry(op, &bo->rx_ops, list) {
if (op->ifindex) {
- /*
- * Only remove subscriptions that had not
- * been removed due to NETDEV_UNREGISTER
- * in bcm_notifier()
- *
- * op->rx_reg_dev is a tracked reference taken
- * when the subscription was registered, so it
- * stays valid here even if a concurrent
- * NETDEV_UNREGISTER already unlisted the device.
+ /* Only remove subscriptions that had not been removed
+ * in bcm_notify(). op->rx_reg_dev keeps the device's
+ * receive lists accessible.
*/
if (op->rx_reg_dev)
bcm_rx_unreg(op->rx_reg_dev, op);
- } else
+ } else {
can_rx_unregister(net, NULL, op->can_id,
REGMASK(op->can_id),
bcm_rx_handler, op);
-
+ }
+ cond_resched();
}
synchronize_rcu();
list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
diff --git a/net/can/isotp.c b/net/can/isotp.c
index da80ef606ce0..649a3446c4f1 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1122,11 +1122,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
u32 my_gen;
u32 old_cfecho;
int off;
int err;
- if (!so->bound || READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN)
+ if (!READ_ONCE(so->bound) || READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN)
return -EADDRNOTAVAIL;
/* claim the socket under so->rx_lock: this serializes the claim
* with the RX path and with sendmsg()'s own error paths below, so
* none of them can ever see a transfer mid-claim
@@ -1167,11 +1167,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
spin_unlock_bh(&so->rx_lock);
/* so->bound is only checked once above - a wakeup may have
* unbound/rebound the socket meanwhile => recheck
*/
- if (!so->bound) {
+ if (!READ_ONCE(so->bound)) {
err = -EADDRNOTAVAIL;
goto err_out_drop;
}
/* PDU size > default => try max_pdu_size */
@@ -1413,11 +1413,11 @@ static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
int ret = 0;
if (flags & ~(MSG_DONTWAIT | MSG_TRUNC | MSG_PEEK | MSG_CMSG_COMPAT))
return -EINVAL;
- if (!so->bound)
+ if (!READ_ONCE(so->bound))
return -EADDRNOTAVAIL;
skb = skb_recv_datagram(sk, flags, &ret);
if (!skb)
return ret;
@@ -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),
@@ -1506,15 +1506,13 @@ static int isotp_release(struct socket *sock)
isotp_rcv_echo, sk);
netdev_put(so->dev, &so->dev_tracker);
}
so->ifindex = 0;
- so->bound = 0;
+ WRITE_ONCE(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,25 +1602,28 @@ 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);
- if (!dev) {
- err = -ENODEV;
- goto out;
- }
- if (!can_get_ml_priv(dev)) {
+ /* 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. Check the device before
+ * publishing it.
+ */
+ rcu_read_lock();
+ dev = dev_get_by_index_rcu(net, addr->can_ifindex);
+ if (!dev || !can_get_ml_priv(dev))
err = -ENODEV;
- goto out_put_dev;
- }
- if (READ_ONCE(dev->mtu) < so->ll.mtu) {
+ else if (READ_ONCE(dev->mtu) < so->ll.mtu)
err = -EINVAL;
- goto out_put_dev;
+ if (!err) {
+ dev_hold(dev);
+ WRITE_ONCE(so->dev, dev);
}
- if (!(dev->flags & IFF_UP))
- notify_enetdown = 1;
+ rcu_read_unlock();
+ if (err)
+ goto out;
ifindex = dev->ifindex;
if (isotp_register_rxid(so)) {
err = can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
@@ -1646,32 +1647,41 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
/* switch to new settings */
so->ifindex = ifindex;
so->rxid = rx_id;
so->txid = tx_id;
- so->bound = 1;
+ WRITE_ONCE(so->bound, 1);
/* 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);
+ /* Either isotp_notify() sees so->dev on NETDEV_DOWN or we see the
+ * cleared IFF_UP, pairs with isotp_notify()
+ */
+ smp_mb();
+ if (!(READ_ONCE(dev->flags) & IFF_UP))
+ notify_enetdown = 1;
+
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);
}
+ release_sock(sk);
+
return err;
}
static int isotp_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
{
@@ -1863,16 +1873,25 @@ 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)
+ if (msg == NETDEV_DOWN)
+ smp_mb(); /* pairs with isotp_bind(), see raw_notify() */
+
+ /* 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),
@@ -1883,20 +1902,23 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
isotp_rcv_echo, sk);
netdev_put(so->dev, &so->dev_tracker);
}
so->ifindex = 0;
- so->bound = 0;
- so->dev = NULL;
- release_sock(sk);
+ WRITE_ONCE(so->bound, 0);
+ WRITE_ONCE(so->dev, NULL);
sk->sk_err = ENODEV;
if (!sock_flag(sk, SOCK_DEAD))
sk_error_report(sk);
+ release_sock(sk);
break;
case NETDEV_DOWN:
+ if (!READ_ONCE(so->bound))
+ break;
+
sk->sk_err = ENETDOWN;
if (!sock_flag(sk, SOCK_DEAD))
sk_error_report(sk);
break;
}
diff --git a/net/can/raw.c b/net/can/raw.c
index dfea48768b23..0267b4300c24 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -83,10 +83,11 @@ struct uniqframe {
struct raw_sock {
struct sock sk;
struct net_device *dev;
netdevice_tracker dev_tracker;
+ struct net_device *bind_dev; /* hint for raw_notify(), no reference */
struct list_head notifier;
int ifindex;
unsigned int bound:1;
unsigned int loopback:1;
unsigned int recv_own_msgs:1;
@@ -222,10 +223,11 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
{
int err = 0;
int i;
for (i = 0; i < count; i++) {
+ cond_resched();
err = can_rx_register(net, dev, filter[i].can_id,
filter[i].can_mask,
raw_rcv, sk, sock_i_ino(sk), sk);
if (err) {
/* clean up successfully registered filters */
@@ -256,13 +258,15 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
struct sock *sk, struct can_filter *filter,
int count)
{
int i;
- for (i = 0; i < count; i++)
+ for (i = 0; i < count; i++) {
can_rx_unregister(net, dev, filter[i].can_id,
filter[i].can_mask, raw_rcv, sk);
+ cond_resched();
+ }
}
static inline void raw_disable_errfilter(struct net *net,
struct net_device *dev,
struct sock *sk,
@@ -303,20 +307,35 @@ static int raw_enable_allfilters(struct net *net, struct net_device *dev,
static void raw_notify(struct raw_sock *ro, unsigned long msg,
struct net_device *dev)
{
struct sock *sk = &ro->sk;
+ struct net_device *bind_dev;
if (!net_eq(dev_net(dev), sock_net(sk)))
return;
- if (ro->dev != dev)
+ if (msg == NETDEV_DOWN)
+ smp_mb(); /* IFF_UP is cleared before, pairs with raw_bind() */
+
+ /* Lockless fast path, ro->bind_dev covers a concurrent bind().
+ * Read it before ro->dev, pairs with smp_store_release() in
+ * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
+ */
+ bind_dev = smp_load_acquire(&ro->bind_dev);
+ if (READ_ONCE(ro->dev) != dev &&
+ (msg != NETDEV_UNREGISTER || bind_dev != dev))
return;
switch (msg) {
case NETDEV_UNREGISTER:
lock_sock(sk);
+ if (ro->dev != dev) {
+ release_sock(sk);
+ break;
+ }
+
/* remove current filters & unregister */
if (ro->bound) {
raw_disable_allfilters(dev_net(dev), dev, sk);
netdev_put(dev, &ro->dev_tracker);
}
@@ -324,17 +343,17 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
if (ro->count > 1)
kfree(ro->filter);
ro->ifindex = 0;
ro->bound = 0;
- ro->dev = NULL;
+ WRITE_ONCE(ro->dev, NULL);
ro->count = 0;
- release_sock(sk);
sk->sk_err = ENODEV;
if (!sock_flag(sk, SOCK_DEAD))
sk_error_report(sk);
+ release_sock(sk);
break;
case NETDEV_DOWN:
sk->sk_err = ENETDOWN;
if (!sock_flag(sk, SOCK_DEAD))
@@ -379,10 +398,11 @@ static int raw_init(struct sock *sk)
struct raw_sock *ro = raw_sk(sk);
ro->bound = 0;
ro->ifindex = 0;
ro->dev = NULL;
+ ro->bind_dev = NULL;
/* set default filter to single entry dfilter */
ro->dfilter.can_id = 0;
ro->dfilter.can_mask = MASK_ALL;
ro->filter = &ro->dfilter;
@@ -429,17 +449,19 @@ static int raw_release(struct socket *sock)
spin_lock(&raw_notifier_lock);
}
list_del(&ro->notifier);
spin_unlock(&raw_notifier_lock);
- rtnl_lock();
lock_sock(sk);
- /* remove current filters & unregister */
+ /* remove current filters & unregister
+ * the tracked reference ro->dev keeps the device's receive lists
+ * accessible, even after a namespace change
+ */
if (ro->bound) {
if (ro->dev) {
- raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
+ raw_disable_allfilters(net, ro->dev, sk);
netdev_put(ro->dev, &ro->dev_tracker);
} else {
raw_disable_allfilters(net, NULL, sk);
}
}
@@ -454,11 +476,10 @@ static int raw_release(struct socket *sock)
sock_orphan(sk);
sock->sk = NULL;
release_sock(sk);
- rtnl_unlock();
sock_prot_inuse_add(net, sk->sk_prot, -1);
sock_put(sk);
return 0;
@@ -477,30 +498,38 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
if (len < RAW_MIN_NAMELEN)
return -EINVAL;
if (addr->can_family != AF_CAN)
return -EINVAL;
- rtnl_lock();
+ /* lock_sock() serializes the binding with raw_notify() */
lock_sock(sk);
if (ro->bound && addr->can_ifindex == ro->ifindex)
goto out;
if (addr->can_ifindex) {
- dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
+ /* Publish the device inside the RCU section of the lookup:
+ * When it is unlisted concurrently, either raw_notify() sees
+ * ro->bind_dev after synchronize_net() or the lookup fails.
+ */
+ rcu_read_lock();
+ dev = dev_get_by_index_rcu(sock_net(sk), addr->can_ifindex);
+ if (dev) {
+ dev_hold(dev);
+ /* pairs with smp_load_acquire() in raw_notify() */
+ smp_store_release(&ro->bind_dev, dev);
+ }
+ rcu_read_unlock();
if (!dev) {
err = -ENODEV;
goto out;
}
if (!can_get_ml_priv(dev)) {
err = -ENODEV;
goto out_put_dev;
}
- if (!(dev->flags & IFF_UP))
- notify_enetdown = 1;
-
ifindex = dev->ifindex;
/* filters set by default/setsockopt */
err = raw_enable_allfilters(sock_net(sk), dev, sk);
if (err)
@@ -515,38 +544,46 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
if (!err) {
if (ro->bound) {
/* unregister old filters */
if (ro->dev) {
- raw_disable_allfilters(dev_net(ro->dev),
+ raw_disable_allfilters(sock_net(sk),
ro->dev, sk);
/* drop reference to old ro->dev */
netdev_put(ro->dev, &ro->dev_tracker);
} else {
raw_disable_allfilters(sock_net(sk), NULL, sk);
}
}
ro->ifindex = ifindex;
ro->bound = 1;
/* bind() ok -> hold a reference for new ro->dev */
- ro->dev = dev;
+ WRITE_ONCE(ro->dev, dev);
netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
+
+ if (dev) {
+ /* Either raw_notify() sees ro->dev on NETDEV_DOWN or
+ * we see the cleared IFF_UP, pairs with raw_notify()
+ */
+ smp_mb();
+ if (!(READ_ONCE(dev->flags) & IFF_UP))
+ notify_enetdown = 1;
+ }
}
out_put_dev:
- /* remove potential reference from dev_get_by_index() */
+ /* 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);
}
+ release_sock(sk);
+
return err;
}
static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
int peer)
@@ -596,12 +633,17 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
} else if (count == 1) {
if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
return -EFAULT;
}
+ /* The reg_state check is only a shortcut: On a concurrent
+ * unregistration or namespace change raw_notify() waits for
+ * lock_sock() and removes the filters set here.
+ */
dev = ro->dev;
- if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED) {
+ if (ro->bound && dev &&
+ READ_ONCE(dev->reg_state) != NETREG_REGISTERED) {
if (count > 1)
kfree(filter);
return -ENODEV;
}
@@ -645,12 +687,14 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
if (copy_from_sockptr(&err_mask, optval, optlen))
return -EFAULT;
err_mask &= CAN_ERR_MASK;
+ /* lockless shortcut, see CAN_RAW_FILTER */
dev = ro->dev;
- if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED)
+ if (ro->bound && dev &&
+ READ_ONCE(dev->reg_state) != NETREG_REGISTERED)
return -ENODEV;
/* remove current error mask */
if (ro->bound) {
/* (try to) register the new err_mask */
@@ -753,17 +797,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
int err;
if (level != SOL_CAN_RAW)
return -EINVAL;
- rtnl_lock();
lock_sock(sk);
err = raw_setsockopt_locked(sock, optname, optval, optlen);
release_sock(sk);
- rtnl_unlock();
return err;
}
static int raw_getsockopt(struct socket *sock, int level, int optname,
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v8 7/7] can: raw, isotp: copy socket options outside of lock_sock()
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (5 preceding siblings ...)
2026-10-23 12:18 ` [PATCH v8 6/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
@ 2026-10-23 12:18 ` Oliver Hartkopp
2026-10-25 11:28 ` [PATCH v8 0/7] can: protect the receive lists with a per-device lock Marc Kleine-Budde
2026-10-25 11:43 ` Oliver Hartkopp
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-23 12:18 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, stable
The CAN_RAW and CAN_ISOTP specific socket options are copied from and
to user space while holding lock_sock(). A copy that faults or stalls,
e.g. on a userfaultfd or FUSE backed mapping, holds lock_sock() for its
whole duration.
Since CAN_RAW and CAN_ISOTP do not take RTNL in bind() anymore,
raw_notify() and isotp_notify() wait for lock_sock() of a socket that
is bound to the vanishing device while holding RTNL. As the CAN_RAW
options can be changed on a bound socket, such a stalled copy can block
RTNL on an unregistration or namespace change of the bound device.
CAN_ISOTP only copies its options for unbound sockets, which
isotp_notify() skips without taking lock_sock().
Keep the user space copies out of the locked sections: Check the option
name and length and copy the option values, including the CAN_RAW_FILTER
filters, to kernel memory before lock_sock(). raw_setsockopt_locked()
and isotp_setsockopt_locked() get the copied values directly and drop
their now redundant length checks and copies. As a side effect, a
failing copy can not leave partially overwritten options behind
anymore. In raw_getsockopt() duplicate the filters under lock_sock() and
copy them to user space after release_sock().
As the option name and length are now checked before lock_sock(), the
order of the checks in isotp_setsockopt() and accordingly the order of
the returned error values changes. This is in line with POSIX, which
does not define an order in which multiple error conditions are
detected and reported.
The generic SOL_SOCKET options in sk_setsockopt() are not affected by
this patch.
Fixes: ee8b94c8510c ("can: raw: fix receiver memory leak")
Cc: stable@vger.kernel.org # depends on "can: protect the receive lists with a per-device lock"
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/isotp.c | 100 +++++++++++++++-----------
net/can/raw.c | 186 +++++++++++++++++++++++++-----------------------
2 files changed, 155 insertions(+), 131 deletions(-)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 649a3446c4f1..ad4996f29007 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1699,27 +1699,23 @@ static int isotp_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
addr->can_addr.tp.tx_id = so->txid;
return ISOTP_MIN_NAMELEN;
}
-static int isotp_setsockopt_locked(struct socket *sock, int level, int optname,
- sockptr_t optval, unsigned int optlen)
+static int isotp_setsockopt_locked(struct socket *sock, int optname,
+ const void *val)
{
struct sock *sk = sock->sk;
struct isotp_sock *so = isotp_sk(sk);
int ret = 0;
if (so->bound)
return -EISCONN;
switch (optname) {
case CAN_ISOTP_OPTS:
- if (optlen != sizeof(struct can_isotp_options))
- return -EINVAL;
-
- if (copy_from_sockptr(&so->opt, optval, optlen))
- return -EFAULT;
+ memcpy(&so->opt, val, sizeof(so->opt));
/* no separate rx_ext_address is given => use ext_address */
if (!(so->opt.flags & CAN_ISOTP_RX_EXT_ADDR))
so->opt.rx_ext_address = so->opt.ext_address;
@@ -1740,59 +1736,41 @@ static int isotp_setsockopt_locked(struct socket *sock, int level, int optname,
so->frame_txtime = so->opt.frame_txtime;
}
break;
case CAN_ISOTP_RECV_FC:
- if (optlen != sizeof(struct can_isotp_fc_options))
- return -EINVAL;
-
- if (copy_from_sockptr(&so->rxfc, optval, optlen))
- return -EFAULT;
+ memcpy(&so->rxfc, val, sizeof(so->rxfc));
break;
case CAN_ISOTP_TX_STMIN:
- if (optlen != sizeof(u32))
- return -EINVAL;
-
- if (copy_from_sockptr(&so->force_tx_stmin, optval, optlen))
- return -EFAULT;
+ so->force_tx_stmin = *(const u32 *)val;
break;
case CAN_ISOTP_RX_STMIN:
- if (optlen != sizeof(u32))
- return -EINVAL;
-
- if (copy_from_sockptr(&so->force_rx_stmin, optval, optlen))
- return -EFAULT;
+ so->force_rx_stmin = *(const u32 *)val;
break;
- case CAN_ISOTP_LL_OPTS:
- if (optlen == sizeof(struct can_isotp_ll_options)) {
- struct can_isotp_ll_options ll;
-
- if (copy_from_sockptr(&ll, optval, optlen))
- return -EFAULT;
+ case CAN_ISOTP_LL_OPTS: {
+ const struct can_isotp_ll_options *ll = val;
- /* check for correct ISO 11898-1 DLC data length */
- if (ll.tx_dl != padlen(ll.tx_dl))
- return -EINVAL;
+ /* check for correct ISO 11898-1 DLC data length */
+ if (ll->tx_dl != padlen(ll->tx_dl))
+ return -EINVAL;
- if (ll.mtu != CAN_MTU && ll.mtu != CANFD_MTU)
- return -EINVAL;
+ if (ll->mtu != CAN_MTU && ll->mtu != CANFD_MTU)
+ return -EINVAL;
- if (ll.mtu == CAN_MTU &&
- (ll.tx_dl > CAN_MAX_DLEN || ll.tx_flags != 0))
- return -EINVAL;
+ if (ll->mtu == CAN_MTU &&
+ (ll->tx_dl > CAN_MAX_DLEN || ll->tx_flags != 0))
+ return -EINVAL;
- memcpy(&so->ll, &ll, sizeof(ll));
+ memcpy(&so->ll, ll, sizeof(*ll));
- /* set ll_dl for tx path to similar place as for rx */
- so->tx.ll_dl = ll.tx_dl;
- } else {
- return -EINVAL;
- }
+ /* set ll_dl for tx path to similar place as for rx */
+ so->tx.ll_dl = ll->tx_dl;
break;
+ }
default:
ret = -ENOPROTOOPT;
}
@@ -1802,17 +1780,53 @@ static int isotp_setsockopt_locked(struct socket *sock, int level, int optname,
static int isotp_setsockopt(struct socket *sock, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
struct sock *sk = sock->sk;
+ union {
+ struct can_isotp_options opt;
+ struct can_isotp_fc_options rxfc;
+ struct can_isotp_ll_options ll;
+ u32 stmin;
+ } val;
+ unsigned int size;
int ret;
if (level != SOL_CAN_ISOTP)
return -EINVAL;
+ /* copy the option values from user space before lock_sock() */
+ switch (optname) {
+ case CAN_ISOTP_OPTS:
+ size = sizeof(val.opt);
+ break;
+
+ case CAN_ISOTP_RECV_FC:
+ size = sizeof(val.rxfc);
+ break;
+
+ case CAN_ISOTP_TX_STMIN:
+ case CAN_ISOTP_RX_STMIN:
+ size = sizeof(val.stmin);
+ break;
+
+ case CAN_ISOTP_LL_OPTS:
+ size = sizeof(val.ll);
+ break;
+
+ default:
+ return -ENOPROTOOPT;
+ }
+
+ if (optlen != size)
+ return -EINVAL;
+
+ if (copy_from_sockptr(&val, optval, size))
+ return -EFAULT;
+
lock_sock(sk);
- ret = isotp_setsockopt_locked(sock, level, optname, optval, optlen);
+ ret = isotp_setsockopt_locked(sock, optname, &val);
release_sock(sk);
return ret;
}
static int isotp_getsockopt(struct socket *sock, int level, int optname,
diff --git a/net/can/raw.c b/net/can/raw.c
index 0267b4300c24..66e41bf9db3e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -601,67 +601,40 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
return RAW_MIN_NAMELEN;
}
static int raw_setsockopt_locked(struct socket *sock, int optname,
- sockptr_t optval, unsigned int optlen)
+ const void *val, struct can_filter *filter,
+ int count)
{
struct sock *sk = sock->sk;
struct raw_sock *ro = raw_sk(sk);
- struct can_filter *filter = NULL; /* dyn. alloc'ed filters */
- struct can_filter sfilter; /* single filter */
struct net_device *dev = NULL;
- can_err_mask_t err_mask = 0;
- int count = 0;
+ can_err_mask_t err_mask;
int flag;
int err = 0;
switch (optname) {
case CAN_RAW_FILTER:
- if (optlen % sizeof(struct can_filter) != 0)
- return -EINVAL;
-
- if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter))
- return -EINVAL;
-
- count = optlen / sizeof(struct can_filter);
-
- if (count > 1) {
- /* filter does not fit into dfilter => alloc space */
- filter = memdup_sockptr(optval, optlen);
- if (IS_ERR(filter))
- return PTR_ERR(filter);
- } else if (count == 1) {
- if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
- return -EFAULT;
- }
-
- /* The reg_state check is only a shortcut: On a concurrent
+ /* @filter has been copied by raw_setsockopt(). It is linked
+ * to the socket on success and freed by the caller on error.
+ *
+ * The reg_state check is only a shortcut: On a concurrent
* unregistration or namespace change raw_notify() waits for
* lock_sock() and removes the filters set here.
*/
dev = ro->dev;
if (ro->bound && dev &&
- READ_ONCE(dev->reg_state) != NETREG_REGISTERED) {
- if (count > 1)
- kfree(filter);
+ READ_ONCE(dev->reg_state) != NETREG_REGISTERED)
return -ENODEV;
- }
if (ro->bound) {
/* (try to) register the new filters */
- if (count == 1)
- err = raw_enable_filters(sock_net(sk), dev, sk,
- &sfilter, 1);
- else
- err = raw_enable_filters(sock_net(sk), dev, sk,
- filter, count);
- if (err) {
- if (count > 1)
- kfree(filter);
+ err = raw_enable_filters(sock_net(sk), dev, sk, filter,
+ count);
+ if (err)
return err;
- }
/* remove old filter registrations */
raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
ro->count);
}
@@ -671,25 +644,19 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
kfree(ro->filter);
/* link new filters to the socket */
if (count == 1) {
/* copy filter data for single filter */
- ro->dfilter = sfilter;
+ ro->dfilter = *filter;
filter = &ro->dfilter;
}
ro->filter = filter;
ro->count = count;
break;
case CAN_RAW_ERR_FILTER:
- if (optlen != sizeof(err_mask))
- return -EINVAL;
-
- if (copy_from_sockptr(&err_mask, optval, optlen))
- return -EFAULT;
-
- err_mask &= CAN_ERR_MASK;
+ err_mask = *(const can_err_mask_t *)val & CAN_ERR_MASK;
/* lockless shortcut, see CAN_RAW_FILTER */
dev = ro->dev;
if (ro->bound && dev &&
READ_ONCE(dev->reg_state) != NETREG_REGISTERED)
@@ -712,78 +679,46 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
/* link new err_mask to the socket */
ro->err_mask = err_mask;
break;
case CAN_RAW_LOOPBACK:
- if (optlen != sizeof(flag))
- return -EINVAL;
-
- if (copy_from_sockptr(&flag, optval, optlen))
- return -EFAULT;
-
- ro->loopback = !!flag;
+ ro->loopback = !!*(const int *)val;
break;
case CAN_RAW_RECV_OWN_MSGS:
- if (optlen != sizeof(flag))
- return -EINVAL;
-
- if (copy_from_sockptr(&flag, optval, optlen))
- return -EFAULT;
-
- ro->recv_own_msgs = !!flag;
+ ro->recv_own_msgs = !!*(const int *)val;
break;
case CAN_RAW_FD_FRAMES:
- if (optlen != sizeof(flag))
- return -EINVAL;
-
- if (copy_from_sockptr(&flag, optval, optlen))
- return -EFAULT;
+ flag = *(const int *)val;
/* Enabling CAN XL includes CAN FD */
if (ro->xl_frames && !flag)
return -EINVAL;
ro->fd_frames = !!flag;
break;
case CAN_RAW_XL_FRAMES:
- if (optlen != sizeof(flag))
- return -EINVAL;
-
- if (copy_from_sockptr(&flag, optval, optlen))
- return -EFAULT;
-
- ro->xl_frames = !!flag;
+ ro->xl_frames = !!*(const int *)val;
/* Enabling CAN XL includes CAN FD */
if (ro->xl_frames)
ro->fd_frames = ro->xl_frames;
break;
case CAN_RAW_XL_VCID_OPTS:
- if (optlen != sizeof(ro->raw_vcid_opts))
- return -EINVAL;
-
- if (copy_from_sockptr(&ro->raw_vcid_opts, optval, optlen))
- return -EFAULT;
+ memcpy(&ro->raw_vcid_opts, val, sizeof(ro->raw_vcid_opts));
/* prepare 32 bit values for handling in hot path */
ro->tx_vcid_shifted = ro->raw_vcid_opts.tx_vcid << CANXL_VCID_OFFSET;
ro->rx_vcid_shifted = ro->raw_vcid_opts.rx_vcid << CANXL_VCID_OFFSET;
ro->rx_vcid_mask_shifted = ro->raw_vcid_opts.rx_vcid_mask << CANXL_VCID_OFFSET;
break;
case CAN_RAW_JOIN_FILTERS:
- if (optlen != sizeof(flag))
- return -EINVAL;
-
- if (copy_from_sockptr(&flag, optval, optlen))
- return -EFAULT;
-
- ro->join_filters = !!flag;
+ ro->join_filters = !!*(const int *)val;
break;
default:
return -ENOPROTOOPT;
}
@@ -792,21 +727,88 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
static int raw_setsockopt(struct socket *sock, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
struct sock *sk = sock->sk;
+ struct can_filter *filter = NULL; /* dyn. alloc'ed filters */
+ struct can_filter sfilter; /* single filter */
+ union {
+ can_err_mask_t err_mask;
+ int flag;
+ struct can_raw_vcid_options vcid_opts;
+ } val;
+ unsigned int size = 0;
+ int count = 0;
int err;
if (level != SOL_CAN_RAW)
return -EINVAL;
+ /* Copy the option values from user space before lock_sock(), so a
+ * stalled copy can not block raw_notify() waiting for lock_sock()
+ * under RTNL.
+ */
+ switch (optname) {
+ case CAN_RAW_FILTER:
+ if (optlen % sizeof(struct can_filter) != 0)
+ return -EINVAL;
+
+ if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter))
+ return -EINVAL;
+
+ count = optlen / sizeof(struct can_filter);
+
+ if (count > 1) {
+ /* filter does not fit into dfilter => alloc space */
+ filter = memdup_sockptr(optval, optlen);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);
+ } else if (count == 1) {
+ if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
+ return -EFAULT;
+ filter = &sfilter;
+ }
+ break;
+
+ case CAN_RAW_ERR_FILTER:
+ size = sizeof(val.err_mask);
+ break;
+
+ case CAN_RAW_XL_VCID_OPTS:
+ size = sizeof(val.vcid_opts);
+ break;
+
+ case CAN_RAW_LOOPBACK:
+ case CAN_RAW_RECV_OWN_MSGS:
+ case CAN_RAW_FD_FRAMES:
+ case CAN_RAW_XL_FRAMES:
+ case CAN_RAW_JOIN_FILTERS:
+ size = sizeof(val.flag);
+ break;
+
+ default:
+ return -ENOPROTOOPT;
+ }
+
+ if (size) {
+ if (optlen != size)
+ return -EINVAL;
+
+ if (copy_from_sockptr(&val, optval, size))
+ return -EFAULT;
+ }
+
lock_sock(sk);
- err = raw_setsockopt_locked(sock, optname, optval, optlen);
+ err = raw_setsockopt_locked(sock, optname, &val, filter, count);
release_sock(sk);
+ /* on error free the filters allocated for CAN_RAW_FILTER above */
+ if (err && count > 1)
+ kfree(filter);
+
return err;
}
static int raw_getsockopt(struct socket *sock, int level, int optname,
sockopt_t *opt)
@@ -823,10 +825,11 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
if (len < 0)
return -EINVAL;
switch (optname) {
case CAN_RAW_FILTER: {
+ struct can_filter *filter = NULL;
int err = 0;
lock_sock(sk);
if (ro->count > 0) {
int fsize = ro->count * sizeof(struct can_filter);
@@ -837,19 +840,26 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
err = -ERANGE;
opt->optlen = fsize;
} else {
if (len > fsize)
len = fsize;
- if (copy_to_iter(ro->filter, len,
- &opt->iter_out) != len)
- err = -EFAULT;
+ /* copy to user space after release_sock() */
+ filter = kmemdup(ro->filter, len, GFP_KERNEL);
+ if (!filter)
+ err = -ENOMEM;
}
} else {
len = 0;
}
release_sock(sk);
+ if (filter) {
+ if (copy_to_iter(filter, len, &opt->iter_out) != len)
+ err = -EFAULT;
+ kfree(filter);
+ }
+
if (!err)
opt->optlen = len;
return err;
}
case CAN_RAW_ERR_FILTER:
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (6 preceding siblings ...)
2026-10-23 12:18 ` [PATCH v8 7/7] can: raw, isotp: copy socket options outside of lock_sock() Oliver Hartkopp
@ 2026-10-25 11:28 ` Marc Kleine-Budde
2026-10-25 18:55 ` Oliver Hartkopp
2026-10-25 11:43 ` Oliver Hartkopp
8 siblings, 1 reply; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 11:28 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 901 bytes --]
On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
> 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].
This series doesn't apply to net-next/main, probably as it's based on
https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
Sashiko fails and you'll get no review feedback.
"b4" has a feature to track dependencies and put annotations into the
series. Please consider switching to b4 and use "b4 prep --edit-deps" to
track dependencies.
regards,
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 11:28 ` [PATCH v8 0/7] can: protect the receive lists with a per-device lock Marc Kleine-Budde
@ 2026-10-25 18:55 ` Oliver Hartkopp
2026-10-25 19:06 ` Marc Kleine-Budde
0 siblings, 1 reply; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-25 18:55 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
On 25.10.26 12:28, Marc Kleine-Budde wrote:
> On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
>> 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].
>
> This series doesn't apply to net-next/main, probably as it's based on
> https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
> Sashiko fails and you'll get no review feedback.
>
> "b4" has a feature to track dependencies and put annotations into the
> series. Please consider switching to b4 and use "b4 prep --edit-deps" to
> track dependencies.
Good idea. But it does not help for my current problem:
https://lore.kernel.org/linux-can/bd31fd1b-1fe7-44a5-99ee-635262a1ed33@hartkopp.net/
Against which tree should I post the patches?
Best regards,
Oliver
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 18:55 ` Oliver Hartkopp
@ 2026-10-25 19:06 ` Marc Kleine-Budde
2026-10-25 19:16 ` Oliver Hartkopp
0 siblings, 1 reply; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 19:06 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]
On 25.10.2026 19:55:11, Oliver Hartkopp wrote:
>
>
> On 25.10.26 12:28, Marc Kleine-Budde wrote:
> > On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
> > > 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].
> >
> > This series doesn't apply to net-next/main, probably as it's based on
> > https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
> > Sashiko fails and you'll get no review feedback.
> >
> > "b4" has a feature to track dependencies and put annotations into the
> > series. Please consider switching to b4 and use "b4 prep --edit-deps" to
> > track dependencies.
>
> Good idea. But it does not help for my current problem:
>
> https://lore.kernel.org/linux-can/bd31fd1b-1fe7-44a5-99ee-635262a1ed33@hartkopp.net/
>
> Against which tree should I post the patches?
Does this work?
base-commit: net-next/main
message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:06 ` Marc Kleine-Budde
@ 2026-10-25 19:16 ` Oliver Hartkopp
2026-10-25 19:19 ` Marc Kleine-Budde
0 siblings, 1 reply; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-25 19:16 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
On 25.10.26 20:06, Marc Kleine-Budde wrote:
> On 25.10.2026 19:55:11, Oliver Hartkopp wrote:
>>
>>
>> On 25.10.26 12:28, Marc Kleine-Budde wrote:
>>> On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
>>>> 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].
>>>
>>> This series doesn't apply to net-next/main, probably as it's based on
>>> https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
>>> Sashiko fails and you'll get no review feedback.
>>>
>>> "b4" has a feature to track dependencies and put annotations into the
>>> series. Please consider switching to b4 and use "b4 prep --edit-deps" to
>>> track dependencies.
>>
>> Good idea. But it does not help for my current problem:
>>
>> https://lore.kernel.org/linux-can/bd31fd1b-1fe7-44a5-99ee-635262a1ed33@hartkopp.net/
>>
>> Against which tree should I post the patches?
>
> Does this work?
>
> base-commit: net-next/main
> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
Ah, I see what you are aiming for.
But net-next does not have the required "can: raw: remove redundant NULL
check before netdev_hold()" commit.
Or did you intend
base-commit: linux-can-next/main
message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
??
Best regards,
Oliver
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:16 ` Oliver Hartkopp
@ 2026-10-25 19:19 ` Marc Kleine-Budde
2026-10-25 19:22 ` Oliver Hartkopp
0 siblings, 1 reply; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 19:19 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]
On 25.10.2026 20:16:58, Oliver Hartkopp wrote:
>
>
> On 25.10.26 20:06, Marc Kleine-Budde wrote:
> > On 25.10.2026 19:55:11, Oliver Hartkopp wrote:
> > >
> > >
> > > On 25.10.26 12:28, Marc Kleine-Budde wrote:
> > > > On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
> > > > > 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].
> > > >
> > > > This series doesn't apply to net-next/main, probably as it's based on
> > > > https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
> > > > Sashiko fails and you'll get no review feedback.
> > > >
> > > > "b4" has a feature to track dependencies and put annotations into the
> > > > series. Please consider switching to b4 and use "b4 prep --edit-deps" to
> > > > track dependencies.
> > >
> > > Good idea. But it does not help for my current problem:
> > >
> > > https://lore.kernel.org/linux-can/bd31fd1b-1fe7-44a5-99ee-635262a1ed33@hartkopp.net/
> > >
> > > Against which tree should I post the patches?
> >
> > Does this work?
> >
> > base-commit: net-next/main
> > message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
>
> Ah, I see what you are aiming for.
>
> But net-next does not have the required "can: raw: remove redundant NULL
> check before netdev_hold()" commit.
ACK
> Or did you intend
>
> base-commit: linux-can-next/main
> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That is "can: raw: remove redundant NULL check before netdev_hold()" and
should add it.
You can test with "b4 prep --check-deps".
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:19 ` Marc Kleine-Budde
@ 2026-10-25 19:22 ` Oliver Hartkopp
2026-10-25 19:43 ` Marc Kleine-Budde
0 siblings, 1 reply; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-25 19:22 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
On 25.10.26 20:19, Marc Kleine-Budde wrote:
> On 25.10.2026 20:16:58, Oliver Hartkopp wrote:
>>
>>
>> On 25.10.26 20:06, Marc Kleine-Budde wrote:
>>> On 25.10.2026 19:55:11, Oliver Hartkopp wrote:
>>>>
>>>>
>>>> On 25.10.26 12:28, Marc Kleine-Budde wrote:
>>>>> On 23.10.2026 14:17:54, Oliver Hartkopp wrote:
>>>>>> 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].
>>>>>
>>>>> This series doesn't apply to net-next/main, probably as it's based on
>>>>> https://lore.kernel.org/all/20260831052031.902699-1-ekffu200098@gmail.com/
>>>>> Sashiko fails and you'll get no review feedback.
>>>>>
>>>>> "b4" has a feature to track dependencies and put annotations into the
>>>>> series. Please consider switching to b4 and use "b4 prep --edit-deps" to
>>>>> track dependencies.
>>>>
>>>> Good idea. But it does not help for my current problem:
>>>>
>>>> https://lore.kernel.org/linux-can/bd31fd1b-1fe7-44a5-99ee-635262a1ed33@hartkopp.net/
>>>>
>>>> Against which tree should I post the patches?
>>>
>>> Does this work?
>>>
>>> base-commit: net-next/main
>>> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
>>
>> Ah, I see what you are aiming for.
>>
>> But net-next does not have the required "can: raw: remove redundant NULL
>> check before netdev_hold()" commit.
>
> ACK
>
>> Or did you intend
>>
>> base-commit: linux-can-next/main
^^^^^^^^^^^^^^^^^^^
My question was about the tree!
I got the message id thing.
>> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> That is "can: raw: remove redundant NULL check before netdev_hold()" and
> should add it.
>
> You can test with "b4 prep --check-deps".
>
> Marc
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:22 ` Oliver Hartkopp
@ 2026-10-25 19:43 ` Marc Kleine-Budde
2026-10-25 19:51 ` Oliver Hartkopp
0 siblings, 1 reply; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 19:43 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 611 bytes --]
On 25.10.2026 20:22:30, Oliver Hartkopp wrote:
> > > base-commit: linux-can-next/main
> ^^^^^^^^^^^^^^^^^^^
> My question was about the tree!
linux-can-next/main is an older version of net-next/main, so this
doesn't matter. Both don't have "can: raw: remove redundant NULL check
before netdev_hold()" applied.
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:43 ` Marc Kleine-Budde
@ 2026-10-25 19:51 ` Oliver Hartkopp
2026-10-25 20:01 ` Marc Kleine-Budde
0 siblings, 1 reply; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-25 19:51 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can
On 25.10.26 20:43, Marc Kleine-Budde wrote:
> On 25.10.2026 20:22:30, Oliver Hartkopp wrote:
>>>> base-commit: linux-can-next/main
>> ^^^^^^^^^^^^^^^^^^^
>> My question was about the tree!
>
> linux-can-next/main is an older version of net-next/main, so this
> doesn't matter. Both don't have "can: raw: remove redundant NULL check
> before netdev_hold()" applied.
So it has to be
base-commit: linux-can-next/testing
message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
??
My current idea would be letting b4 fork the branch for my patches in
linux-can-next/testing at commit 0c35fa862dab ("can: proc: remove
pointers from CAN specific proc output") where "can: replace ARPHRD_CAN
type checks with can_get_ml_priv()" would be the next commit (and the
start of my diverting patch set).
Would this work?
Best regards,
Oliver
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 19:51 ` Oliver Hartkopp
@ 2026-10-25 20:01 ` Marc Kleine-Budde
2026-10-25 20:02 ` Marc Kleine-Budde
0 siblings, 1 reply; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 20:01 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]
On 25.10.2026 20:51:56, Oliver Hartkopp wrote:
>
>
> On 25.10.26 20:43, Marc Kleine-Budde wrote:
> > On 25.10.2026 20:22:30, Oliver Hartkopp wrote:
> > > > > base-commit: linux-can-next/main
> > > ^^^^^^^^^^^^^^^^^^^
> > > My question was about the tree!
> >
> > linux-can-next/main is an older version of net-next/main, so this
> > doesn't matter. Both don't have "can: raw: remove redundant NULL check
> > before netdev_hold()" applied.
> So it has to be
>
> base-commit: linux-can-next/testing
> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
No, please use:
base-commit: net-next/main
message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
and check:
| $ b4 prep --check-deps
| Checking base-commit: net-next/main
| Checking message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
| Looking up 20260831052031.902699-1-ekffu200098@gmail.com
| Breaking thread to remove parents of 20260831052031.902699-1-ekffu200098@gmail.com
| Testing if all patches can be applied to net-next/main
| Looking up 20260831052031.902699-1-ekffu200098@gmail.com
| [PATCH] can: raw: remove redundant NULL check before netdev_hold()
| [PATCH v9 1/7] can: proc: reset pkg_stats atomics individually
| [PATCH v9 2/7] can: proc: skip stats calculation after reset in can_stat_update()
| [PATCH v9 3/7] can: proc: remove pointers from CAN specific proc output
| [PATCH v9 4/7] can: replace ARPHRD_CAN type checks with can_get_ml_priv()
| [PATCH v9 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind()
| [PATCH v9 6/7] can: protect the receive lists with a per-device lock
| [PATCH v9 7/7] can: raw, isotp: copy socket options outside of lock_sock()
| Magic: Preparing a sparse worktree
| ---
| ● message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
| ● base-commit: net-next/main
| ● base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main
> My current idea would be letting b4 fork the branch for my patches in
> linux-can-next/testing at commit 0c35fa862dab ("can: proc: remove pointers
> from CAN specific proc output") where "can: replace ARPHRD_CAN type checks
> with can_get_ml_priv()" would be the next commit (and the start of my
> diverting patch set).
>
> Would this work?
My idea (see above, now tested) is to use net-next/main and apply
20260831052031.902699-1-ekffu200098@gmail.com as the dependencies of
your patches.
regards,
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-25 20:01 ` Marc Kleine-Budde
@ 2026-10-25 20:02 ` Marc Kleine-Budde
0 siblings, 0 replies; 19+ messages in thread
From: Marc Kleine-Budde @ 2026-10-25 20:02 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: linux-can
[-- Attachment #1: Type: text/plain, Size: 3025 bytes --]
On 25.10.2026 21:01:15, Marc Kleine-Budde wrote:
> On 25.10.2026 20:51:56, Oliver Hartkopp wrote:
> >
> >
> > On 25.10.26 20:43, Marc Kleine-Budde wrote:
> > > On 25.10.2026 20:22:30, Oliver Hartkopp wrote:
> > > > > > base-commit: linux-can-next/main
> > > > ^^^^^^^^^^^^^^^^^^^
> > > > My question was about the tree!
> > >
> > > linux-can-next/main is an older version of net-next/main, so this
> > > doesn't matter. Both don't have "can: raw: remove redundant NULL check
> > > before netdev_hold()" applied.
> > So it has to be
> >
> > base-commit: linux-can-next/testing
> > message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
>
> No, please use:
>
> base-commit: net-next/main
> message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
>
> and check:
>
> | $ b4 prep --check-deps
> | Checking base-commit: net-next/main
> | Checking message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
> | Looking up 20260831052031.902699-1-ekffu200098@gmail.com
> | Breaking thread to remove parents of 20260831052031.902699-1-ekffu200098@gmail.com
> | Testing if all patches can be applied to net-next/main
> | Looking up 20260831052031.902699-1-ekffu200098@gmail.com
> | [PATCH] can: raw: remove redundant NULL check before netdev_hold()
> | [PATCH v9 1/7] can: proc: reset pkg_stats atomics individually
> | [PATCH v9 2/7] can: proc: skip stats calculation after reset in can_stat_update()
> | [PATCH v9 3/7] can: proc: remove pointers from CAN specific proc output
> | [PATCH v9 4/7] can: replace ARPHRD_CAN type checks with can_get_ml_priv()
> | [PATCH v9 5/7] can: isotp: check the return values of can_rx_register() in isotp_bind()
> | [PATCH v9 6/7] can: protect the receive lists with a per-device lock
> | [PATCH v9 7/7] can: raw, isotp: copy socket options outside of lock_sock()
> | Magic: Preparing a sparse worktree
> | ---
> | ● message-id: <20260831052031.902699-1-ekffu200098@gmail.com>
> | ● base-commit: net-next/main
> | ● base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main
>
> > My current idea would be letting b4 fork the branch for my patches in
> > linux-can-next/testing at commit 0c35fa862dab ("can: proc: remove pointers
> > from CAN specific proc output") where "can: replace ARPHRD_CAN type checks
> > with can_get_ml_priv()" would be the next commit (and the start of my
> > diverting patch set).
> >
> > Would this work?
>
> My idea (see above, now tested) is to use net-next/main and apply
> 20260831052031.902699-1-ekffu200098@gmail.com as the dependencies of
> your patches.
For illustration purpose, I've pushed my b4/can-per-device-receive-lists
branch to linux-can-next.
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: 228 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v8 0/7] can: protect the receive lists with a per-device lock
2026-10-23 12:17 [PATCH v8 0/7] can: protect the receive lists with a per-device lock Oliver Hartkopp
` (7 preceding siblings ...)
2026-10-25 11:28 ` [PATCH v8 0/7] can: protect the receive lists with a per-device lock Marc Kleine-Budde
@ 2026-10-25 11:43 ` Oliver Hartkopp
8 siblings, 0 replies; 19+ messages in thread
From: Oliver Hartkopp @ 2026-10-25 11:43 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can@vger.kernel.org
Hi Marc,
this latest patch set does not apply to any of those 7 "Baseline" repos
tested by sashiko:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023121801.3052075-1-socketcan%40hartkopp.net
The patches sashiko had problems with were
patch 4: can: replace ARPHRD_CAN type checks with can_get_ml_priv()
patch 5: can: protect the receive lists with a per-device lock
As requested I added two new patches addressing sahikos reports, which
increases this series from 5 to 7 patches:
patch 4: can: replace ARPHRD_CAN type checks with can_get_ml_priv()
patch 5: can: isotp: check the return values of can_rx_register() in
isotp_bind()
patch 6: can: protect the receive lists with a per-device lock
patch 7: can: raw, isotp: copy socket options outside of lock_sock()
My question: How can we get a sashiko review for this before sending a
new net PR?
One idea would be to set linux-can-next/HEAD to contain the last commit
before "can: replace ARPHRD_CAN type checks with can_get_ml_priv()":
b4b56efd3e30 ("can: proc: remove pointers from CAN specific proc output")
so that I would send a v9 patch set containing only the 4 updated
patches (currently counted 4 .. 7) for review.
Best regards,
Oliver
On 23.10.26 14:17, Oliver Hartkopp wrote:
> 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].
>
> Patch 5 makes isotp_bind() check the return values of can_rx_register(),
> so that bind() does not succeed with missing receivers.
>
> Patch 7 copies the CAN_RAW and CAN_ISOTP specific socket options from
> and to user space outside of lock_sock(), so that a stalled copy can not
> block raw_notify() waiting for lock_sock() under RTNL.
>
> The code of patches 1-4 is unchanged. Only the commit message of patch 4
> has been updated after the Sashiko review of v4 to describe the actual
> NULL pointer dereference in can_rx_unregister() and the CAN_ISOTP path
> that reaches it.
>
> The series is based on linux-can-next/testing, which also contains
> "can: raw: remove redundant NULL check before netdev_hold()" from [2].
>
> The idea behind patch 6:
>
> 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 6 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 CAN_RAW, CAN_BCM and CAN_ISOTP 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(). As RTNL does not
> keep NETDEV_DOWN out of bind() anymore, bind() checks IFF_UP after
> publishing the bound device, with an smp_mb() pair against the
> notifiers, so a down transition during bind() is still reported with
> ENETDOWN.
>
> Changes in v8:
> - new patch 5: check the return values of can_rx_register() in
> isotp_bind() (Sashiko review)
> - patch 6 rebased on patch 5: isotp_bind() resets the published so->dev
> again when the registration of a receiver fails and checks IFF_UP at
> the end of a successful bind(), as raw_bind() does
> - patch 6: isotp_notify() reports NETDEV_DOWN only for a bound socket, so
> a failing isotp_bind() can not get a needless ENETDOWN
> - patch 6: isotp_sendmsg() and isotp_recvmsg() read so->bound with
> READ_ONCE()
> - patch 6: moved copying the user space data outside of lock_sock() in
> raw_setsockopt() to patch 7 to keep patch 6 focused on the locking
> changes and to ease review
> - new patch 7: copy the CAN_RAW and CAN_ISOTP socket options outside of
> lock_sock() (Sashiko review)
>
> Changes in v7 (details in patch 6):
> - report ENETDOWN in bind() and ENODEV for NETDEV_UNREGISTER under
> lock_sock(), so ENODEV can not be overwritten by ENETDOWN (Sashiko
> review)
>
> Changes in v6:
> - isotp_bind() checks the device before publishing so->dev, so a
> failing bind() can not get a needless ENETDOWN (Sashiko review)
>
> Changes in v5:
> - report ENETDOWN for a down transition during bind() again (Sashiko
> review)
> - comment and kernel-doc updates (Sashiko review)
> - patch 4: commit message describes the actual crash path (Sashiko
> review)
>
> [1] https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> [2] https://lore.kernel.org/linux-can/20261018164417.1540449-1-mkl@pengutronix.de/
>
> Previous versions of patch 6:
> v7: https://lore.kernel.org/linux-can/20261022114202.180023-6-socketcan@hartkopp.net/
> v6: https://lore.kernel.org/linux-can/20261021231130.2085690-12-mkl@pengutronix.de/
> v5: https://lore.kernel.org/linux-can/20261020203257.230955-6-socketcan@hartkopp.net/
> v4: https://lore.kernel.org/linux-can/20261019152741.54854-6-socketcan@hartkopp.net/
> v3: https://lore.kernel.org/linux-can/20261019074244.50391-5-socketcan@hartkopp.net/
> v2: https://lore.kernel.org/linux-can/20261018180739.689750-1-socketcan@hartkopp.net/
> v1: https://lore.kernel.org/linux-can/20261017201935.399233-1-socketcan@hartkopp.net/
>
> Oliver Hartkopp (6):
> can: proc: skip stats calculation after reset in can_stat_update()
> can: proc: remove pointers from CAN specific proc output
> can: replace ARPHRD_CAN type checks with can_get_ml_priv()
> can: isotp: check the return values of can_rx_register() in
> isotp_bind()
> can: protect the receive lists with a per-device lock
> can: raw, isotp: copy socket options outside of lock_sock()
>
> 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 | 60 ++++---
> net/can/af_can.h | 2 +-
> net/can/bcm.c | 64 +++----
> net/can/gw.c | 9 +-
> net/can/isotp.c | 206 ++++++++++++++---------
> net/can/j1939/main.c | 2 +-
> net/can/proc.c | 95 ++++++-----
> net/can/raw.c | 278 ++++++++++++++++++-------------
> 12 files changed, 427 insertions(+), 312 deletions(-)
>
^ permalink raw reply [flat|nested] 19+ messages in thread