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

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]/

>
> 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 don't recall why exactly it was needed but Im afraid hdev->send_cmd
needs to match NOP, we do use HCI_OP_NOP to wait on an event without
sending a request.

> 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
>


-- 
Luiz Augusto von Dentz