On 2021/07/08 8:33, Tetsuo Handa wrote:
>> we could perhaps don't release the reference to hdev
>> either and leave hci_sock_release to deal with it and then perhaps we
>> can take away the backward goto, actually why are you restarting to
>> begin with?
>
> Do you mean something like
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index b04a5a02ecf3..0525883f4639 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -759,19 +759,14 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
> if (event == HCI_DEV_UNREG) {
> struct sock *sk;
>
> - /* Detach sockets from device */
> + /* Change socket state and notify */
> read_lock(&hci_sk_list.lock);
> sk_for_each(sk, &hci_sk_list.head) {
> - lock_sock(sk);
> if (hci_pi(sk)->hdev == hdev) {
> - hci_pi(sk)->hdev = NULL;
> sk->sk_err = EPIPE;
> sk->sk_state = BT_OPEN;
> sk->sk_state_change(sk);
> -
> - hci_dev_put(hdev);
> }
> - release_sock(sk);
> }
> read_unlock(&hci_sk_list.lock);
> }
>
> ? I can't judge because I don't know how this works. I worry that
> without lock_sock()/release_sock(), this races with e.g. hci_sock_bind().
>
I examined hci_unregister_dev() and concluded that this can't work.
hci_sock_dev_event(hdev, HCI_DEV_UNREG) can't defer dropping the reference to
this hdev till hci_sock_release(), for hci_unregister_dev() cleans up everything
related to this hdev and calls hci_dev_put(hdev) and then vhci_release() calls
hci_free_dev(hdev).
That's the reason hci_sock_dev_event() has to use lock_sock() in order not to
miss some hci_dev_put(hdev) calls.
>> This sounds a little too complicated, afaik backward goto is not even
>> consider a good practice either, since it appears we don't unlink the
>> sockets here
Despite your comment, I'd like to go with choice (3) for now. After lock_sock() became
free from delay caused by pagefault handling, we could consider updating to choice (1).
