can: bcm: fix stale rx/tx ops after device removal

RX: an RX_SETUP update(!) for an existing op skipped can_rx_register()
unconditionally, even when a concurrent NETDEV_UNREGISTER had already
torn down its registration (op->rx_reg_dev == NULL). This silently
did not re-enable frame delivery for that updated filter. bcm_rx_setup()
now re-registers in that case, while leaving rx_ops with ifindex = 0
(all CAN devices) which never carry a tracked rx_reg_dev registered as-is.

TX: bcm_notify() only handled bo->rx_ops on NETDEV_UNREGISTER, leaving
tx_ops with an active cyclic transmission re-arming its hrtimer
indefinitely to execute bcm_tx_timeout_handler(). Cancelling the hrtimer
prevents the runaway timer and any injection into a later reused ifindex,
since nothing else calls bcm_can_tx() for the op until an explicit
TX_SETUP update re-arms it.

Unlike bcm_rx_unreg(), which clears the tracked rx_reg_dev for rx_ops,
the ifindex is intentionally left unchanged for tx_ops. bcm_tx_setup()
always rejects ifindex 0, so clearing it would strand the op: neither a
later TX_SETUP (bcm_find_op()) nor TX_DELETE (bcm_delete_tx_op()) could
ever find it again, since both require an exact ifindex match.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-can/20260708094536.DDF821F00A3A@smtp.kernel.org/
Closes: https://lore.kernel.org/linux-can/20260708154039.347ED1F000E9@smtp.kernel.org/
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260714-bcm_fixes-v15-9-562f7e3e42da@hartkopp.net
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 25842061800ba1baf952593fb91d17a822579f39..a53dba6ab8b8901259b4d72e95b9880501d8be94 100644 (file)
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1287,6 +1287,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
        struct bcm_sock *bo = bcm_sk(sk);
        struct bcm_op *op;
        int do_rx_register;
+       int new_op = 0;
        int err = 0;
 
        if ((msg_head->flags & RX_FILTER_ID) || (!(msg_head->nframes))) {
@@ -1371,8 +1372,15 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                /* free temporary frames / kfree(NULL) is safe */
                kfree(new_frames);
 
-               /* Only an update -> do not call can_rx_register() */
-               do_rx_register = 0;
+               /* Don't register a new CAN filter for the rx_op update unless
+                * a concurrent NETDEV_UNREGISTER notifier already tore down
+                * the previous registration. In this case the receiver needs
+                * to be re-registered here so that this update doesn't
+                * silently stop delivering frames for the given ifindex.
+                * Ops with ifindex = 0 (all CAN interfaces) never carry a
+                * tracked rx_reg_dev and stay registered as-is.
+                */
+               do_rx_register = (ifindex && !op->rx_reg_dev) ? 1 : 0;
 
        } else {
                /* insert new BCM operation for the given can_id */
@@ -1439,6 +1447,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
                /* call can_rx_register() */
                do_rx_register = 1;
+               new_op = 1;
 
        } /* if ((op = bcm_find_op(&bo->rx_ops, msg_head->can_id, ifindex))) */
 
@@ -1452,7 +1461,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                if (op->flags & SETTIMER) {
 
                        /* set timers (locked) for newly created op */
-                       if (do_rx_register) {
+                       if (new_op) {
                                spin_lock_bh(&op->bcm_rx_update_lock);
                                op->ival1 = msg_head->ival1;
                                op->ival2 = msg_head->ival2;
@@ -1482,7 +1491,10 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                                      HRTIMER_MODE_REL_SOFT);
        }
 
-       /* now we can register for can_ids, if we added a new bcm_op */
+       /* now we can register for can_ids, if we added a new bcm_op
+        * or need to re-register after a NETDEV_UNREGISTER tore down
+        * the previous registration of an existing op
+        */
        if (do_rx_register) {
                if (ifindex) {
                        struct net_device *dev;
@@ -1514,18 +1526,32 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
                                err = -ENODEV;
                        }
 
-               } else
+               } else {
                        err = can_rx_register(sock_net(sk), NULL, op->can_id,
                                              REGMASK(op->can_id),
                                              bcm_rx_handler, op, "bcm", sk);
+               }
+
                if (err) {
-                       /* this bcm rx op is broken -> remove it */
-                       bcm_remove_op(op);
+                       /* newly created bcm rx op is broken -> remove it */
+                       if (new_op) {
+                               bcm_remove_op(op);
+                               return err;
+                       }
+
+                       /* an existing op just stays unregistered.
+                        * Cancel op->timer and (defensively) op->thrtimer.
+                        * Other settings can't be reached until the next
+                        * successful RX_SETUP.
+                        */
+                       hrtimer_cancel(&op->timer);
+                       hrtimer_cancel(&op->thrtimer);
                        return err;
                }
 
-               /* add this bcm_op to the list of the rx_ops */
-               list_add_rcu(&op->list, &bo->rx_ops);
+               /* add a new bcm_op to the list of the rx_ops */
+               if (new_op)
+                       list_add_rcu(&op->list, &bo->rx_ops);
        }
 
        return msg_head->nframes * op->cfsiz + MHSIZ;
@@ -1745,11 +1771,19 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
        case NETDEV_UNREGISTER:
                lock_sock(sk);
 
-               /* remove device specific receive entries */
+               /* rx_ops: remove device specific receive entries */
                list_for_each_entry(op, &bo->rx_ops, list)
                        if (op->rx_reg_dev == dev)
                                bcm_rx_unreg(dev, op);
 
+               /* tx_ops: stop device specific cyclic transmissions on the
+                * vanishing ifindex. Cancelling the timer is enough to stop
+                * cyclic bcm_can_tx() calls as there is no re-arming.
+                */
+               list_for_each_entry(op, &bo->tx_ops, list)
+                       if (op->ifindex == dev->ifindex)
+                               hrtimer_cancel(&op->timer);
+
                /* remove device reference, if this is our bound device */
                if (bo->bound && bo->ifindex == dev->ifindex) {
 #if IS_ENABLED(CONFIG_PROC_FS)