Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210717000731.3836303-1-luiz.dentz@gmail.com>
 <7a0405c7-63ba-3e5c-7b22-503d7b326a11@i-love.sakura.ne.jp>
 <CABBYNZLdYXHxD47xO+TX6U=MK1RtvuAAHLYvS1vTsuk4nFQfrg@mail.gmail.com> <166a8c6d-9928-4edf-8082-111e555f5808@i-love.sakura.ne.jp>
In-Reply-To: <166a8c6d-9928-4edf-8082-111e555f5808@i-love.sakura.ne.jp>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Sat, 17 Jul 2021 23:51:39 -0700
Message-ID: <CABBYNZJpeRmkB6zGUVDJYTLEFvds8z_ag++vC-mT8faig31O+w@mail.gmail.com>
Subject: Re: [RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        LinMa <linma@zju.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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