Hi Tetsuo, On Sat, Jul 17, 2021 at 11:22 PM Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote: > > On 2021/07/18 14:16, Luiz Augusto von Dentz wrote: > > Anyway after all these discussion Im even > > more convinced that the real problem lies in hci_dev_get/hold, after > > all references are usually used to prevent the objects to be freed but > > in this case it doesn't and no locking will gonna fix that. > > If hci_dev_hold() calls atomic_long_add_unless(&file->f_count, 1, 0) under RCU, > vhci_release(file) would not be called until all sockets using that hdev drops > the reference, and hci_sock_dev_event(hdev, HCI_DEV_UNREG) no longer needs to > traverse sockets on hci_sk_list.head list. This requires adding "struct file *" to > "struct hci_dev". My patch keeps changes be confined to only hci_sock_dev_event(). Being confined doesn't mean that it simple, your changes are doing a loop locking, and I also didn't touch hci_dev_hold because it would affect all drivers but if there is a way to do it by all means we should do it, but notice that we do need a way to cleanup if the device is unregistered so I don't think holding the file directly would be a good idea since it prevents release but it would also prevent cleanup, in other words if the process which open the vhci terminates or close, all bluetooth sockets should receive a proper error so we cannot really change this behavior. From the brief look at it I think we should remove the function hci_dev_free and leave hci_dev_unregister to cleanup everything, but I'm afraid there could be extra references that are not being cleanup properly and finding out where could take a lot more time, well even with your suggestion that could be a problem since we also would need to inspect every time we hold a reference in the same manner. -- Luiz Augusto von Dentz
