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

[Norbert Szetei]

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.

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
>