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
.
