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

Hi,

On 19-Aug-26 18:42, Luiz Augusto von Dentz wrote:
> Hi Hans,
> 
> On Wed, Aug 19, 2026 at 8:38 AM Hans de Goede
> <[email protected]> wrote:
>>
>> 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() ?
> 
> Looks like we found a much better alternative which is to use skb_get
> rather then skb_clone for req_skb:
> 
> https://patchwork.kernel.org/project/bluetooth/patch/[email protected]/

Nice, ok this patch can be dropped from the queue then.

Regards,

Hans