Re: [PATCH net] can: isotp: take rtnl_lock() before leaving the notifier list

[Oliver Hartkopp]

Hi Norbert!

On 01.09.26 09:01, Norbert Szetei wrote:
> Hey Oliver,
>
> > On Aug 31, 2026, at 15:08, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> >
> > Hello Norbert,
> >
> > many thanks for your patch and the analysis of the unremoved filter lists in the case of moving a CAN device to another namespace.
> >
> > But I don't think that moving rtnl_lock() up so that it covers a busy loop including a schedule_timeout_uninterruptible(1) wait is not a nice move for other rtnl_lock() users.
> >
> > Focussing on the removal of the correct filter lists when the namespace is changed away from the socket's namespace I would propose this small change:
> >
> > diff --git a/net/can/isotp.c b/net/can/isotp.c
> > index 155530aedce2..0835a4758a72 100644
> > --- a/net/can/isotp.c
> > +++ b/net/can/isotp.c
> > @@ -1490,15 +1490,15 @@ static int isotp_release(struct socket *sock)
> >         /* remove current filters & unregister
> >          * tracked reference so->dev is taken at bind() time with rtnl_lock
> >          */
> >         if (so->bound && so->dev) {
> >                 if (isotp_register_rxid(so))
> > -                       can_rx_unregister(net, so->dev, so->rxid,
> > +                       can_rx_unregister(dev_net(so->dev), so->dev, so->rxid,
> >                                           SINGLE_MASK(so->rxid),
> >                                           isotp_rcv, sk);
> >
> > -               can_rx_unregister(net, so->dev, so->txid,
> > +               can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
> >                                   SINGLE_MASK(so->txid),
> >                                   isotp_rcv_echo, sk);
> >                 netdev_put(so->dev, &so->dev_tracker);
> >         }
> >
> > @@ -1846,13 +1846,10 @@ static int isotp_getsockopt(struct socket *sock, int level, int optname,
> > static void isotp_notify(struct isotp_sock *so, unsigned long msg,
> >                          struct net_device *dev)
> > {
> >         struct sock *sk = &so->sk;
> >
> > -       if (!net_eq(dev_net(dev), sock_net(sk)))
> > -               return;
> > -
> >         if (so->dev != dev)
> >                 return;
> >
> >         switch (msg) {
> >         case NETDEV_UNREGISTER:
> >
> >
> > Can you give it a try with your KASAN setup and maybe also ask opus about my idea?
>
> I just tested your version and I was no longer able to reproduce
> the bug. Initially, I considered it too, but moving rtnl_lock()
> sounded simpler and I had not thought about the busy-wait sitting
> there. Thanks for pointing this out and submitting the patch.

Thanks for testing!

Btw. sashiko bot pointed out some inconvenience with the removed 
net_eq() check, as I'm checking for ifindex equality in bcm.c at some 
places - and the ifindex values are not unique over all namespaces like 
the struct netdev *dev pointer.

https://lore.kernel.org/linux-can/20260831212432.6C2B51F000E9@xxxxxxxxxxxxxxx/

So I need to extend bcm.c in a way that it is checking the dev pointers 
instead of dev->ifindex in those places. There will be a v2 soon.

Btw. many thanks for testing that the original root cause was fixed with 
this approach.

Best regards,
Oliver


> Regards,
> Norbert
>
> > Many thanks,
> > Oliver
> >
> > On 31.08.26 10:30, Norbert Szetei wrote:
> > > isotp_release() removes the socket from isotp_notifier_list before it
> > > takes rtnl_lock(). The netdev notifier chain runs under RTNL, so a
> > > socket that leaves the list in that window is skipped by isotp_notify()
> > > and has to unregister its own CAN filters.
> > > It cannot always do that. isotp_release() passes sock_net(sk) to
> > > can_rx_unregister(), which returns early when that netns no longer
> > > matches dev_net(dev), before the receiver list is searched and before
> > > the "receive list entry not found" warning. Once the bound device has
> > > been moved to another netns the filters are removed zero times, and
> > > can_rx_register() stores rcv->sk without taking a reference, so the
> > > receivers left in the device's dev_rcv_lists point at the freed socket
> > > and travel with the device into the new netns.
> > >    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
> > > Take rtnl_lock() before removing the socket from the notifier list, so
> > > that isotp_release() and isotp_notify() cannot both skip the removal.
> > > Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Assisted-by: Claude:claude-opus-5
> > > Signed-off-by: Norbert Szetei <norbert@xxxxxxxxxxxx>
> > > ---
> > > Reproducer available on request.
> > >   net/can/isotp.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > diff --git a/net/can/isotp.c b/net/can/isotp.c
> > > index 155530aedce2..8ca75d30360c 100644
> > > --- a/net/can/isotp.c
> > > +++ b/net/can/isotp.c
> > > @@ -1475,6 +1475,8 @@ static int isotp_release(struct socket *sock)
> > >    /* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */
> > >    wake_up_interruptible(&so->wait);
> > >   + rtnl_lock();
> > > +
> > >    spin_lock(&isotp_notifier_lock);
> > >    while (isotp_busy_notifier == so) {
> > >    spin_unlock(&isotp_notifier_lock);
> > > @@ -1484,7 +1486,6 @@ static int isotp_release(struct socket *sock)
> > >    list_del(&so->notifier);
> > >    spin_unlock(&isotp_notifier_lock);
> > >   - rtnl_lock();
> > >    lock_sock(sk);
> > >      /* remove current filters & unregister