Re: [RFC PATCH] Bluetooth: fix Set Public Address on controller in HCI_AUTO_OFF grace period

[Dan Klishch <danilklishch@gmail.com>]

Hi Luiz,

On 5/4/26 2:27 PM, Luiz Augusto von Dentz wrote:
> Hi Dan,
> 
> On Thu, Apr 30, 2026 at 12:55 AM Dan Klishch <danilklishch@gmail.com> wrote:
>>
>> When mgmt's Set Public Address command (and Set External Configuration)
>> stages a configuration change, set_public_address() sets HCI_CONFIG and
>> HCI_AUTO_OFF on the device and queues hci_power_on. The intent is for
>> the queued power_on to run hci_dev_init_sync() -- which detects
>> HCI_CONFIG and calls hdev->set_bdaddr() to program the new address into
>> the firmware -- and then re-emit Index Added with the controller back in
>> the boot-init grace period (HCI_UP=1, HCI_AUTO_OFF=1) so userspace can
>> re-push pending config commands and drive a fresh Set Powered 1 cycle.
>>
>> The bug bites when the command is issued while the controller is in
>> that same grace period itself -- the normal post-boot state, before
>> bluetoothd has claimed the controller via Set Powered 1. In that state
>> HCI_UP=1 and HCI_AUTO_OFF=1, so hdev_is_powered() is false (the command
>> is accepted) but hci_power_on()'s "already up" early-return condition
>> HCI_UP && HCI_MGMT && HCI_AUTO_OFF is true. The early-return path runs
>> hci_powered_update_sync() -- which is the wrong thing for this case:
>> neither hci_dev_init_sync() nor mgmt_index_added() runs. The result:
>>
>>    - hdev->public_addr is recorded but never reaches the firmware via
>>      hdev->set_bdaddr(),
>>    - userspace sees Index Removed (from set_public_address) but no
>>      Index Added, leaving the controller invisible to mgmt clients;
>>      it is not in the configured, unconfigured or extended index list
>>      yet remains registered in the kernel.
>>
>> The other two starting states are fine: HCI_UP=0 falls through to the
>> full hci_dev_do_open() path (correct); HCI_UP=1 with HCI_AUTO_OFF=0
>> makes hdev_is_powered() true, so set_public_address() rejects with
>> MGMT_STATUS_REJECTED before queueing power_on at all.
>>
>> Fix: at the top of hci_power_on(), if HCI_CONFIG is pending and we are
>> in the affected grace-period state (HCI_UP, HCI_MGMT, !HCI_RFKILLED),
>> close the device first. The early-return condition then fails and we
>> fall through to hci_dev_do_open() -> hci_dev_init_sync(), which honors
>> HCI_CONFIG by invoking hdev->set_bdaddr(); the post-open block then
>> re-emits Index Added via the existing HCI_CONFIG branch.
>>
>> hci_dev_close_sync() clears HCI_AUTO_OFF as a side effect of going
>> through the regular power-down path, but set_public_address() had set
>> it deliberately so the post-reopen state matches the boot-init grace
>> period that mgmt clients expect ("treat as new one" per the protocol
>> contract). Several mgmt commands -- Set Privacy, Set Wideband Speech,
>> Set LE, Set BR/EDR, etc. -- gate on !hdev_is_powered(), and bluetoothd
>> expects to push them during this window before issuing Set Powered 1
>> itself. Restore HCI_AUTO_OFF after the close so hdev_is_powered() ==
>> false again.
>>
>> This matches the documented behavior for Set Public Address on a
>> fully-configured controller ("Once the address has been successfully
>> changed an Index Added event will be sent.")
>>
>> Assisted-by: Claude:claude-opus-4-7
>> Signed-off-by: Dan Klishch <danilklishch@gmail.com>
>> ---
>>   net/bluetooth/hci_core.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -948,6 +948,14 @@ static void hci_power_on(struct work_struct *work)
>>
>>          BT_DBG("%s", hdev->name);
>>
>> +       if (hci_dev_test_flag(hdev, HCI_CONFIG) &&
>> +           hci_dev_test_flag(hdev, HCI_MGMT) &&
>> +           !hci_dev_test_flag(hdev, HCI_RFKILLED) &&
>> +           test_bit(HCI_UP, &hdev->flags)) {
>> +               hci_dev_do_close(hdev);
>> +               hci_dev_set_flag(hdev, HCI_AUTO_OFF);
>> +       }
>> +
>>          if (test_bit(HCI_UP, &hdev->flags) &&
>>              hci_dev_test_flag(hdev, HCI_MGMT) &&
>>              hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF)) {
>>
>> ---
>>
>> Claude was able to fully convince me that the patch and the explanation
>> are correct. Moreover, this indeed fixed a problem I had with a custom
>> patched bluetoothd that happened to override public-addr in the 2
>> second post-boot window. However, both the patch and commit description
>> are fully AI-generated.
> 
> This doesn't inspire much confidence, though. Have you at least tried
> it yourself? Perhaps we need to ask Claude to first add a test to
> mgmt-tester to ensure it actually fixes something rather than just
> hallucinating. Most models are not actually very critical of their
> proposals if there is nothing to validate their claims with.
> 

I did test this indeed. The bluez patch I mentioned:
https://gist.github.com/DanShaders/4d280096a5f0f2f45ce07028cbd36761
(this is yet another AI slop, sorry!; I just wanted something quick for
myself). Without the kernel patch, MGMT_OP_SET_PUBLIC_ADDRESS is just
silently swallowed, adapter is removed, and then the userspace never
sees it again.

Claude says it cannot easily add an in-tree test for the patch without
additional changes in vhci to support setting public address. Should I
tell it to do a 3 commit series (set public addr on vhci; this; a test)
and send as V2?

One more question: seems like CI is angry on me because I included the
`Assisted-by: Claude:claude-opus-4-7` tag as per
https://docs.kernel.org/process/coding-assistants.html . Should I remove
it?

(Resending since I accidentally chose "Reply" instead of "Reply All" the
first time, I'm not very used to mailing lists)