Re: [PATCH 2/2] Bluetooth: hci_core: Handle skb_clone() failure in hci_send_cmd_sync()

Hi Luiz,

On 18-Aug-26 20:01, Luiz Augusto von Dentz wrote:
> Hi Hans, Ibrahim,
> 
> On Mon, Aug 17, 2026 at 4:50 PM Hans de Goede
> <[email protected]> wrote:
>>
>> From: Ibrahim Abdelkader <[email protected]>
>>
>> hci_send_cmd_sync() does not handle the second skb_clone() failure as it
>> does the first one. At that point the command has already been sent and
>> HCI_CMD_PENDING is set, so an allocation failure leaves the flag set with
>> req_skb NULL. The response then matches nothing and the caller blocks for
>> HCI_INIT_TIMEOUT before getting -ETIMEDOUT, reporting a memory shortage as
>> a controller timeout.
>>
>> Clear the flag, cancel the pending request with -ENOMEM so the caller fails
>> immediately instead of waiting out the timeout, and return -ENOMEM so
>> hci_cmd_work() does not arm cmd_timer for a request that no longer exists.
>>
>> Clearing HCI_CMD_PENDING here is safe: all code touching that flag runs
>> from hdev->workqueue, which is an ordered workqueue, so those paths are
>> serialised and cannot race. The one exception is the cleanup added by
>> commit ed516e3f1aa6 ("Bluetooth: hci_sync: Clear HCI_CMD_PENDING when
>> dropping the last request"), which runs on hci_dev_open_sync() failure and
>> from hci_dev_close_sync(), and the workqueue is stopped before that runs.
>>
>> Fixes: 2615fd9a7c25 ("Bluetooth: hci_sync: Fix overwriting request callback")
>> Signed-off-by: Ibrahim Abdelkader <[email protected]>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>>  net/bluetooth/hci_core.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 8aeb3024428b..9585b42e7970 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -4092,6 +4092,11 @@ static int hci_send_cmd_sync(struct hci_dev *hdev, struct sk_buff *skb)
>>             !hci_dev_test_and_set_flag(hdev, HCI_CMD_PENDING)) {
>>                 kfree_skb(hdev->req_skb);
>>                 hdev->req_skb = skb_clone(hdev->sent_cmd, GFP_KERNEL);
>> +               if (!hdev->req_skb) {
>> +                       hci_dev_clear_flag(hdev, HCI_CMD_PENDING);
>> +                       hci_cmd_sync_cancel_sync(hdev, ENOMEM);
>> +                       err = -ENOMEM;
>> +               }
> 
> Sashiko flagged a problem with this change:
> 
> https://sashiko.dev/#/patchset/20260817205017.56524-1-johannes.goede%40oss.qualcomm.com
> 
> Im with the opinion that this shouldn't cancel the request, because at
> this point it may already been sent to the hardware so maybe we should
> flag it somehow that req_skb wasn't able to be allocated, or something
> like that, but the command is still outstanding.

Ok, so maybe we should move the hdev->req_skb = skb_clone(hdev->sent_cmd, ...);
to above hci_send_frame() ?  I don't really see a reason why that cannot
be done first.

And then the hdev->req_skb = skb_clone(hdev->sent_cmd, ...) and
hci_send_frame() error handling could share a common err path both
calling hci_cmd_sync_cancel_sync() ?


Also while looking at this I noticed that hci_send_cmd_sync() exits with
ENODATA when hci_skb_opcode(skb) == HCI_OP_NOP, but at this point it
has already set hdev->sent_cmd and will also still set hdev->req_skb
if the conditions for that are met.

I wonder if this is the right thing to do, or if the hci_skb_opcode(skb) ==
HCI_OP_NOP check should be moved higher up in the function followed by
an immediate kfree_skb(skb) + return -ENODATA ?

Regards,

Hans