Re: [RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Tetsuo,

On Sat, Jul 17, 2021 at 11:22 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> 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