[PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report

[PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report

[Zumeng Chen]

This patch is to balance the condition scope between hci_get_cmd_complete and
hci_event_packet about orig_skb as follows: 

        if (req_complete_skb || event == HCI_EV_CMD_STATUS ||
            event == HCI_EV_CMD_COMPLETE)
                orig_skb = skb_clone(skb, GFP_KERNEL);

And hci_get_cmd_complete will bt_dev_err out when HCI_EV_CMD_STATUS, so a lot
of asymmetric conditions are triggered. Since both of them are the entry into
hci_get_cmd_complete, we'd better get STATUS into judge the false out there.

Signed-off-by: Zumeng Chen <zumeng.chen@gmail.com>
---

Hi expert,

This issue existed whether or not T_DBG had been changed into bt_dev_err, which
just shows the issue explicitly. I noticed actually that opcode doesn't match
ev->opcode either at the same time. And there might be some logic issue about
HCI_EV_CMD_COMPLETE between protocol and drivers. I'm not familar with the whole
bluetooth protocol, and not gonna to dig more, so all yours guys~

Cheers,
Zumeng

 net/bluetooth/hci_event.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 235b5aa..d848663 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
 		return true;
 	}
 
-	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
+	if (!((hdr->evt == HCI_EV_CMD_COMPLETE) ||
+		(hdr->evt == HCI_EV_CMD_STATUS))) {
 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
 			   hdr->evt);
 		return false;
-- 
2.7.4

Re: [PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report

[Marcel Holtmann]

Hi Zumeng,

> This patch is to balance the condition scope between hci_get_cmd_complete and
> hci_event_packet about orig_skb as follows: 
> 
>        if (req_complete_skb || event == HCI_EV_CMD_STATUS ||
>            event == HCI_EV_CMD_COMPLETE)
>                orig_skb = skb_clone(skb, GFP_KERNEL);
> 
> And hci_get_cmd_complete will bt_dev_err out when HCI_EV_CMD_STATUS, so a lot
> of asymmetric conditions are triggered. Since both of them are the entry into
> hci_get_cmd_complete, we'd better get STATUS into judge the false out there.
> 
> Signed-off-by: Zumeng Chen <zumeng.chen@gmail.com>
> ---
> 
> Hi expert,
> 
> This issue existed whether or not T_DBG had been changed into bt_dev_err, which
> just shows the issue explicitly. I noticed actually that opcode doesn't match
> ev->opcode either at the same time. And there might be some logic issue about
> HCI_EV_CMD_COMPLETE between protocol and drivers. I'm not familar with the whole
> bluetooth protocol, and not gonna to dig more, so all yours guys~
> 
> Cheers,
> Zumeng
> 
> net/bluetooth/hci_event.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 235b5aa..d848663 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
> 		return true;
> 	}
> 
> -	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
> +	if (!((hdr->evt == HCI_EV_CMD_COMPLETE) ||
> +		(hdr->evt == HCI_EV_CMD_STATUS))) {

this indentation is messed up. Also some braces are not needed.

	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
	      hdr->evt == HCI_EV_CMD_STATUS)) {

> 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
> 			   hdr->evt);
> 		return false;

Regards

Marcel

[PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Zumeng Chen]

This patch is to balance the condition scope between hci_get_cmd_complete and
hci_event_packet about orig_skb as follows:

        if (req_complete_skb || event == HCI_EV_CMD_STATUS ||
            event == HCI_EV_CMD_COMPLETE)
                orig_skb = skb_clone(skb, GFP_KERNEL);

And hci_get_cmd_complete will bt_dev_err out when HCI_EV_CMD_STATUS, so a lot
of asymmetric conditions are triggered. Since both of them are the entry into
hci_get_cmd_complete, we'd better get STATUS into judge the false out there.

Signed-off-by: Zumeng Chen <zumeng.chen@gmail.com>
---

v2: remove redundant braces and adjust the indentation.

Cheers,
Zumeng

 net/bluetooth/hci_event.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 235b5aa..1d2a8fe 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
 		return true;
 	}
 
-	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
+	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
+	      hdr->evt == HCI_EV_CMD_STATUS)) {
 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
 			   hdr->evt);
 		return false;
-- 
2.7.4

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Luiz Augusto von Dentz]

Hi Marcel,
On Thu, Nov 15, 2018 at 3:37 AM Zumeng Chen <zumeng.chen@gmail.com> wrote:
>
> This patch is to balance the condition scope between hci_get_cmd_complete and
> hci_event_packet about orig_skb as follows:
>
>         if (req_complete_skb || event == HCI_EV_CMD_STATUS ||
>             event == HCI_EV_CMD_COMPLETE)
>                 orig_skb = skb_clone(skb, GFP_KERNEL);
>
> And hci_get_cmd_complete will bt_dev_err out when HCI_EV_CMD_STATUS, so a lot
> of asymmetric conditions are triggered. Since both of them are the entry into
> hci_get_cmd_complete, we'd better get STATUS into judge the false out there.
>
> Signed-off-by: Zumeng Chen <zumeng.chen@gmail.com>
> ---
>
> v2: remove redundant braces and adjust the indentation.
>
> Cheers,
> Zumeng
>
>  net/bluetooth/hci_event.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 235b5aa..1d2a8fe 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
>                 return true;
>         }
>
> -       if (hdr->evt != HCI_EV_CMD_COMPLETE) {
> +       if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
> +             hdr->evt == HCI_EV_CMD_STATUS)) {
>                 bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
>                            hdr->evt);
>                 return false;
> --
> 2.7.4

It appears we need this also for enabling vendor_diag with intel controllers:

[399314.236288] hci_cmd_status_evt:3138: hci0 opcode 0xfc43
[399314.236291] Bluetooth: hci0: last event is not cmd complete (0x0f)
[399314.236359] Bluetooth: hci0: Changing Intel diagnostic mode failed (-16)


-- 
Luiz Augusto von Dentz

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Johan Hedberg]

Hi,


> On 15 Nov 2018, at 3.31, Zumeng Chen <zumeng.chen@gmail.com> wrote:
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
> 		return true;
> 	}
> 
> -	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
> +	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
> +	      hdr->evt == HCI_EV_CMD_STATUS)) {
> 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
> 			   hdr->evt);
> 		return false;

This is not correct. The purpose of this function is to retrieve the command complete parameters, or the parameters of a specific event if the sending code indicated it (it didn’t in this case). Since the event was not command complete the right behaviour for this function is to fail, i.e. return false. The only issue here is the bt_dev_err, which should probably be downgraded to a BT_DBG. In fact, that’s what it used to be in the past - I’m not sure why it was changed to bt_dev_err.

Johan

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Johan Hedberg]

On 27 Nov 2018, at 11.24, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> On 15 Nov 2018, at 3.31, Zumeng Chen <zumeng.chen@gmail.com> wrote:
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
>> 		return true;
>> 	}
>> 
>> -	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
>> +	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
>> +	      hdr->evt == HCI_EV_CMD_STATUS)) {
>> 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
>> 			   hdr->evt);
>> 		return false;
> 
> This is not correct. The purpose of this function is to retrieve the command complete parameters, or the parameters of a specific event if the sending code indicated it (it didn’t in this case). Since the event was not command complete the right behaviour for this function is to fail, i.e. return false. The only issue here is the bt_dev_err, which should probably be downgraded to a BT_DBG. In fact, that’s what it used to be in the past - I’m not sure why it was changed to bt_dev_err.

The one improvement I’d make however, is to silently return from the function in case of a Command Status event, since that just means that the request is complete, however there are no extra parameters to be retrieved. I’ll be sending a patch for that in a moment.

Johan

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Zumeng Chen]

On 2018年11月27日 17:34, Johan Hedberg wrote:
> On 27 Nov 2018, at 11.24, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>>> On 15 Nov 2018, at 3.31, Zumeng Chen <zumeng.chen@gmail.com> wrote:
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
>>> 		return true;
>>> 	}
>>>
>>> -	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
>>> +	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
>>> +	      hdr->evt == HCI_EV_CMD_STATUS)) {
>>> 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
>>> 			   hdr->evt);
>>> 		return false;
>> This is not correct. The purpose of this function is to retrieve the command complete parameters, or the parameters of a specific event if the sending code indicated it (it didn’t in this case). Since the event was not command complete the right behaviour for this function is to fail, i.e. return false. The only issue here is the bt_dev_err, which should probably be downgraded to a BT_DBG. In fact, that’s what it used to be in the past - I’m not sure why it was changed to bt_dev_err.
> The one improvement I’d make however, is to silently return from the function in case of a Command Status event, since that just means that the request is complete, however there are no extra parameters to be retrieved. I’ll be sending a patch for that in a moment.

There is a related kernel as shown in the below list

https://bugzilla.kernel.org/show_bug.cgi?id=198699

I didn't think my patch is to fix this one because I think we need to 
sort out
the whole logic of CMD_COMPLETE. But my patch is good to fix the issue
described by subject.

Cheers,
Zumeng
>
> Johan
>

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Johan Hedberg]

Hi Zumeng,


> On 27 Nov 2018, at 14.22, Zumeng Chen <zumeng.chen@gmail.com> wrote:
> There is a related kernel as shown in the below list
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=198699
> 
> I didn't think my patch is to fix this one because I think we need to sort out
> the whole logic of CMD_COMPLETE. But my patch is good to fix the issue
> described by subject.

I’m still failing to see any major problem with command complete handling. This seems more related to sending HCI commands that do not complete in Command Complete (such as Inquiry or connection creation) through the hci_request framework together with hci_req_run_skb(). This could either happen by directly using the hci_req_sync() API, such as in the case of the legacy inquiry ioctl, or by some code using hci_cmd_sync() to send a command that does not complete in Command Complete. In these cases you end up with a Command Status with status == 0, which the hci_request code interprets as completion of the request, but is unable to fetch any return parameters from it (which for these cases is fine).

Since what I've describe above is normal usage of the hci_req_sync() and hci_cmd_sync() APIs I still don’t see any other issue except that an error log was thrown rather than than (at most) a debug log. So I’d still go for the patch that I submitted earlier today.

Johan

Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report

[Zumeng Chen]

On 2018年11月27日 17:24, Johan Hedberg wrote:
> Hi,
>
>
>> On 15 Nov 2018, at 3.31, Zumeng Chen <zumeng.chen@gmail.com> wrote:
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -5217,7 +5217,8 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev, u16 opcode,
>> 		return true;
>> 	}
>>
>> -	if (hdr->evt != HCI_EV_CMD_COMPLETE) {
>> +	if (!(hdr->evt == HCI_EV_CMD_COMPLETE ||
>> +	      hdr->evt == HCI_EV_CMD_STATUS)) {
>> 		bt_dev_err(hdev, "last event is not cmd complete (0x%2.2x)",
>> 			   hdr->evt);
>> 		return false;
> This is not correct. The purpose of this function is to retrieve the command complete parameters, or the parameters of a specific event if the sending code indicated it (it didn’t in this case). Since the event was not command complete the right behaviour for this function is to fail, i.e. return false.

Ha, John, I'm pretty sure you do not debug these codes here by yourself, 
but one
point is right, the name has a little confusion.

>   The only issue here is the bt_dev_err, which should probably be downgraded to a BT_DBG. In fact, that’s what it used to be in the past - I’m not sure why it was changed to bt_dev_err.

It has nothing to do with bt_dev_err, BT_DBG just mutes the issues. 
whether or not
it will go wrong there if your driver level didn't take care of some 
events. Actually it's
OK to ignore it for most BT modules.

Cheers,
Zumeng
>
> Johan
>