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

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Norbert!

On 01.09.26 09:01, Norbert Szetei wrote:
> Hey Oliver,
> 
>> On Aug 31, 2026, at 15:08, Oliver Hartkopp <socketcan@hartkopp.net> 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@smtp.kernel.org/

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@vger.kernel.org
>>> Assisted-by: Claude:claude-opus-5
>>> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
>>> ---
>>> 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
>>
>