All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>, Lin Ma <linma@zju.edu.cn>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	"linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Subject: Re: [PATCH v2] Bluetooth: call lock_sock() outside of spinlock section
Date: Sat, 10 Jul 2021 22:34:29 +0900	[thread overview]
Message-ID: <9771b40f-b544-a2a7-04e1-eddb38a4aae7@i-love.sakura.ne.jp> (raw)
In-Reply-To: <79694c01-b69e-a039-6860-d7e612fbc008@i-love.sakura.ne.jp>

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).


  parent reply	other threads:[~2021-07-10 13:34 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-27 13:11 [PATCH] Bluetooth: call lock_sock() outside of spinlock section Tetsuo Handa
2021-06-27 14:05 ` bluez.test.bot
2021-07-07  9:43 ` [PATCH v2] " Tetsuo Handa
2021-07-07 10:08   ` [v2] " bluez.test.bot
2021-07-07 18:20   ` [PATCH v2] " Luiz Augusto von Dentz
2021-07-07 23:33     ` Tetsuo Handa
2021-07-08  1:00       ` LinMa
2021-07-09 13:50         ` Tetsuo Handa
2021-07-10 13:34       ` Tetsuo Handa [this message]
2021-07-08  7:16   ` [v2] " bluez.test.bot
2021-07-13 11:27   ` [PATCH v3] " Tetsuo Handa
2021-07-13 11:57     ` [v3] " bluez.test.bot
2021-07-14 19:20     ` [PATCH v3] " Luiz Augusto von Dentz
2021-07-15  3:03       ` LinMa
2021-07-16  3:47         ` Desmond Cheong Zhi Xi
2021-07-16  4:11           ` Desmond Cheong Zhi Xi
2021-07-16 14:48             ` Tetsuo Handa
2021-07-16 15:26               ` LinMa
2021-07-17 15:41                 ` Yet Another Patch for CVE-2021-3573 LinMa
2021-07-17 15:45                   ` LinMa
2021-07-22  9:36                 ` [PATCH v3] Bluetooth: call lock_sock() outside of spinlock section Tetsuo Handa
2021-07-22  4:47               ` LinMa
2021-07-22  5:16                 ` Tetsuo Handa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9771b40f-b544-a2a7-04e1-eddb38a4aae7@i-love.sakura.ne.jp \
    --to=penguin-kernel@i-love.sakura.ne.jp \
    --cc=davem@davemloft.net \
    --cc=johan.hedberg@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linma@zju.edu.cn \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.