Re: [PATCH 3/3] Bluetooth: Add missing hci_dev locking when calling mgmt functions

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Andrei,

On Tue, Nov 08, 2011, Andrei Emeltchenko wrote:
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -549,8 +549,11 @@ int hci_dev_open(__u16 dev)
> >                hci_dev_hold(hdev);
> >                set_bit(HCI_UP, &hdev->flags);
> >                hci_notify(hdev, HCI_DEV_UP);
> > -               if (!test_bit(HCI_SETUP, &hdev->flags))
> > +               if (!test_bit(HCI_SETUP, &hdev->flags)) {
> > +                       hci_dev_lock_bh(hdev);
> >                        mgmt_powered(hdev, 1);
> > +                       hci_dev_unlock_bh(hdev);
> 
> Shall we acquire lock before test_bit here and below?

Once the HCI_SETUP flag has been cleared it never gets set again for a
hci_dev, so in the above case it doesn't really make a difference
whether we lock before or after the test.

> > @@ -1561,8 +1566,11 @@ void hci_unregister_dev(struct hci_dev *hdev)
> >                kfree_skb(hdev->reassembly[i]);
> >
> >        if (!test_bit(HCI_INIT, &hdev->flags) &&
> > -                                       !test_bit(HCI_SETUP, &hdev->flags))
> > +                                       !test_bit(HCI_SETUP, &hdev->flags)) {
> > +               hci_dev_lock_bh(hdev);
> >                mgmt_index_removed(hdev);
> > +               hci_dev_unlock_bh(hdev);
> > +       }

The same applies for the above. The only question there is about
HCI_INIT. Since we're inside hci_unregister_dev and have already called
hci_dev_do_close I don't think there's any problem here. Furthermore,
the whole test for HCI_INIT looks unnecessary to me since testing for
HCI_SETUP should be enough. I might send a separate patch for that later
(it's anyway unrelated to this locking patch).

Johan