netdev - Re: [PATCH net] can: raw: fix receiver memory leak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 7 Jul 2023 08:22:43 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>,
 mkl@...gutronix.de, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, penguin-kernel@...ove.SAKURA.ne.jp
Subject: Re: [PATCH net] can: raw: fix receiver memory leak



On 07.07.23 03:59, Ziyang Xuan (William) wrote:
>> On 06.07.23 14:48, Ziyang Xuan (William) wrote:
>>
>> (..)
>>
>>>>>         }
>>>>>        out:
>>>>>         release_sock(sk);
>>>>> +    rtnl_unlock();
>>>>
>>>> Would it also fix the issue when just adding the rtnl_locks to raw_bind() and raw_release() as suggested by you?
>>>
>>> This patch just add rtnl_lock in raw_bind() and raw_release(). raw_setsockopt() has rtnl_lock before this. raw_notify()
>>> is under rtnl_lock. My patch has been tested and solved the issue before send. I don't know if it answered your doubts.
>>
>> My question was whether adding rtnl_locks to raw_bind() and raw_release() would be enough to fix the issue.
>>
>> Without introducing the additional ro->dev element!?
> 
> Understand. Just add rtnl_lock to raw_bind() and raw_release() can not fix the issue. I tested.
> 
> We should understand that unregister a net device is divided into two stages generally.
> Fistly, call unregister_netdevice_many() to remove net_dev from device list and add
> net_dev to net_todo_list. Secondly, free net_dev in netdev_run_todo().
> 
> In my issue. Firstly, unregister_netdevice_many() removed can_dev from device
> list and added can_dev to net_todo_list. Then got NULL by dev_get_by_index()
> and receivers in dev_rcv_lists would not be freed in raw_release().
> After raw_release(), ro->bound would be set 0. When NETDEV_UNREGISTER event
> arrived raw_notify(), receivers in dev_rcv_lists would not be freed too
> because ro->bound was already 0. Thus receivers in dev_rcv_lists would be leaked.

Thanks for the clarification and the testing!

I really assumed rtnl_lock would do this job and also protect the entire 
sequence starting with unregister_netdevice_many() !?!

Looking forward to the V2 patch then.

Many thanks,
Oliver

> 
>               cpu0                                        cpu1
> unregister_netdevice_many(can_dev)
>    unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this
>    net_set_todo(can_dev)
> 						raw_release(can_socket)
> 						  dev = dev_get_by_index(, ro->ifindex); // dev == NULL
> 						  if (dev) { // receivers in dev_rcv_lists not free because dev is NULL
> 						    raw_disable_allfilters(, dev, );
> 						    dev_put(dev);
> 						  }
> 						...
> 						ro->bound = 0;
> 						...
> 
> netdev_wait_allrefs_any()
>    call_netdevice_notifiers(NETDEV_UNREGISTER, )
>      raw_notify(, NETDEV_UNREGISTER, )
>        if (ro->bound) // invalid because ro->bound has been set 0
>          raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed
> 
> 
> Thanks,
> William Xuan
> 
>>
>> Best regards,
>> Oliver
>> .