Re: [PATCH] hci: fix double free in hci_req_sync

[Markus Elfring]

…
> +++ b/net/bluetooth/hci_request.c
> @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode,
>  		hdev->req_result = result;
>  		hdev->req_status = HCI_REQ_DONE;
>  		if (skb) {
> -			kfree_skb(hdev->req_skb);
> -			hdev->req_skb = skb_get(skb);
> +			hci_req_skb_release_and_set(hdev, skb_get(skb));
>  		}
>  		wake_up_interruptible(&hdev->req_wait_q);
…

How do you think about to omit any curly brackets here?


…
> +++ b/net/bluetooth/hci_request.h
> @@ -28,6 +28,15 @@
>
>  #define hci_req_sync_lock(hdev)   mutex_lock(&hdev->req_lock)
>  #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock)
> +#define hci_req_skb_release_and_set(hdev, val)		\
> +({							\
> +	if (hdev->req_skb) {				\
> +		spin_lock(&hdev->req_skb_lock);		\
> +		kfree_skb(hdev->req_skb);		\
> +		hdev->req_skb = val;			\
> +		spin_unlock(&hdev->req_skb_lock);	\
> +	}						\
> +})
…

* Do you expect that any data synchronisation should be performed
  for the shown pointer check?

* Can it eventually matter to implement such a macro with a statement
  like “guard(spinlock)(&hdev->req_skb_lock);”?
  https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/spinlock.h#L561


Regards,
Markus