Re: [PATCH v2] Bluetooth: call lock_sock() outside of spinlock section
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: 2021-07-10 13:34:56
Also in:
netdev
On 2021/07/08 8:33, Tetsuo Handa wrote:
quoted hunk ↗ jump to hunk
quoted
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 likediff --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.
quoted
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).