Skip to content

Commit d599482

Browse files
hartkoppmarckleinebudde
authored andcommitted
can: bcm: add missing device refcount for CAN filter removal
sashiko-bot remarked a problem with a concurrent device unregistration in isotp.c which also is present in the bcm.c code. A former fix for raw.c commit c275a17 ("can: raw: add missing refcount for memory leak fix") introduced a netdevice_tracker which solves the issue for bcm.c too. bcm_release(), bcm_delete_rx_op() and bcm_notifier() relied on dev_get_by_index(ifindex) to re-find the device for an rx_op before unregistering its filter. If a concurrent NETDEV_UNREGISTER has already unlisted the device from the ifindex table, that lookup fails and can_rx_unregister() is silently skipped, leaving a stale CAN filter pointing at the soon-to-be-freed bcm_op/socket. Hold a netdev_hold()/netdev_put() tracked reference on op->rx_reg_dev from the moment the rx filter is registered in bcm_rx_setup() until it is unregistered in bcm_rx_unreg(), and use that reference directly in bcm_release() and bcm_delete_rx_op() instead of re-looking the device up by ifindex. Reported-by: sashiko-bot@kernel.org Closes: https://sashiko.dev/#/patchset/20260707094716.63578-1-socketcan@hartkopp.net Fixes: ffd980f ("[CAN]: Add broadcast manager (bcm) protocol") Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> Link: https://patch.msgid.link/20260714-bcm_fixes-v15-8-562f7e3e42da@hartkopp.net Cc: stable@kernel.org Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
1 parent 62ec41f commit d599482

1 file changed

Lines changed: 27 additions & 20 deletions

File tree

net/can/bcm.c

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ struct bcm_op {
128128
struct canfd_frame last_sframe;
129129
struct sock *sk;
130130
struct net_device *rx_reg_dev;
131+
netdevice_tracker rx_reg_dev_tracker;
131132
spinlock_t bcm_tx_lock; /* protect tx data and timer updates */
132133
spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
133134
};
@@ -937,6 +938,7 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
937938

938939
/* mark as removed subscription */
939940
op->rx_reg_dev = NULL;
941+
netdev_put(dev, &op->rx_reg_dev_tracker);
940942
} else
941943
printk(KERN_ERR "can-bcm: bcm_rx_unreg: registered device "
942944
"mismatch %p %p\n", op->rx_reg_dev, dev);
@@ -967,17 +969,14 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
967969
* Only remove subscriptions that had not
968970
* been removed due to NETDEV_UNREGISTER
969971
* in bcm_notifier()
972+
*
973+
* op->rx_reg_dev is a tracked reference taken
974+
* when the subscription was registered, so it
975+
* stays valid here even if a concurrent
976+
* NETDEV_UNREGISTER already unlisted the dev.
970977
*/
971-
if (op->rx_reg_dev) {
972-
struct net_device *dev;
973-
974-
dev = dev_get_by_index(sock_net(op->sk),
975-
op->ifindex);
976-
if (dev) {
977-
bcm_rx_unreg(dev, op);
978-
dev_put(dev);
979-
}
980-
}
978+
if (op->rx_reg_dev)
979+
bcm_rx_unreg(op->rx_reg_dev, op);
981980
} else
982981
can_rx_unregister(sock_net(op->sk), NULL,
983982
op->can_id,
@@ -1496,7 +1495,17 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
14961495
bcm_rx_handler, op,
14971496
"bcm", sk);
14981497

1499-
op->rx_reg_dev = dev;
1498+
/* keep a tracked reference so that a later
1499+
* unregister can safely reach the device even
1500+
* if a concurrent NETDEV_UNREGISTER has
1501+
* already unlisted it by ifindex
1502+
*/
1503+
if (!err) {
1504+
op->rx_reg_dev = dev;
1505+
netdev_hold(dev,
1506+
&op->rx_reg_dev_tracker,
1507+
GFP_KERNEL);
1508+
}
15001509
dev_put(dev);
15011510
} else {
15021511
/* the requested device is gone - do not
@@ -1873,16 +1882,14 @@ static int bcm_release(struct socket *sock)
18731882
* Only remove subscriptions that had not
18741883
* been removed due to NETDEV_UNREGISTER
18751884
* in bcm_notifier()
1885+
*
1886+
* op->rx_reg_dev is a tracked reference taken
1887+
* when the subscription was registered, so it
1888+
* stays valid here even if a concurrent
1889+
* NETDEV_UNREGISTER already unlisted the device.
18761890
*/
1877-
if (op->rx_reg_dev) {
1878-
struct net_device *dev;
1879-
1880-
dev = dev_get_by_index(net, op->ifindex);
1881-
if (dev) {
1882-
bcm_rx_unreg(dev, op);
1883-
dev_put(dev);
1884-
}
1885-
}
1891+
if (op->rx_reg_dev)
1892+
bcm_rx_unreg(op->rx_reg_dev, op);
18861893
} else
18871894
can_rx_unregister(net, NULL, op->can_id,
18881895
REGMASK(op->can_id),

0 commit comments

Comments
 (0)