linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report
@ 2018-11-08  5:47 Zumeng Chen
  2018-11-14  7:54 ` Marcel Holtmann
  0 siblings, 1 reply; 9+ messages in thread
From: Zumeng Chen @ 2018-11-08  5:47 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: marcel, johan.hedberg, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-08  5:47 [PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report Zumeng Chen
@ 2018-11-14  7:54 ` Marcel Holtmann
  2018-11-15  1:31   ` [PATCH 1/1 v2] " Zumeng Chen
  0 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2018-11-14  7:54 UTC (permalink / raw)
  To: Zumeng Chen; +Cc: linux-bluetooth, Johan Hedberg, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-14  7:54 ` Marcel Holtmann
@ 2018-11-15  1:31   ` Zumeng Chen
  2018-11-26 11:23     ` Luiz Augusto von Dentz
  2018-11-27  9:24     ` Johan Hedberg
  0 siblings, 2 replies; 9+ messages in thread
From: Zumeng Chen @ 2018-11-15  1:31 UTC (permalink / raw)
  To: marcel; +Cc: linux-bluetooth, johan.hedberg, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-15  1:31   ` [PATCH 1/1 v2] " Zumeng Chen
@ 2018-11-26 11:23     ` Luiz Augusto von Dentz
  2018-11-27  9:24     ` Johan Hedberg
  1 sibling, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2018-11-26 11:23 UTC (permalink / raw)
  To: zumeng.chen
  Cc: Marcel Holtmann, linux-bluetooth, Johan Hedberg,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-15  1:31   ` [PATCH 1/1 v2] " Zumeng Chen
  2018-11-26 11:23     ` Luiz Augusto von Dentz
@ 2018-11-27  9:24     ` Johan Hedberg
  2018-11-27  9:34       ` Johan Hedberg
  2018-11-27 12:13       ` Zumeng Chen
  1 sibling, 2 replies; 9+ messages in thread
From: Johan Hedberg @ 2018-11-27  9:24 UTC (permalink / raw)
  To: Zumeng Chen; +Cc: linux-bluetooth

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-27  9:24     ` Johan Hedberg
@ 2018-11-27  9:34       ` Johan Hedberg
  2018-11-27 12:22         ` Zumeng Chen
  2018-11-27 12:13       ` Zumeng Chen
  1 sibling, 1 reply; 9+ messages in thread
From: Johan Hedberg @ 2018-11-27  9:34 UTC (permalink / raw)
  To: Zumeng Chen; +Cc: linux-bluetooth

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-27  9:24     ` Johan Hedberg
  2018-11-27  9:34       ` Johan Hedberg
@ 2018-11-27 12:13       ` Zumeng Chen
  1 sibling, 0 replies; 9+ messages in thread
From: Zumeng Chen @ 2018-11-27 12:13 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: linux-bluetooth

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
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-27  9:34       ` Johan Hedberg
@ 2018-11-27 12:22         ` Zumeng Chen
  2018-11-27 21:19           ` Johan Hedberg
  0 siblings, 1 reply; 9+ messages in thread
From: Zumeng Chen @ 2018-11-27 12:22 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: linux-bluetooth

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
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1 v2] Bluetooth: make the balance of judgement condition to fix a false report
  2018-11-27 12:22         ` Zumeng Chen
@ 2018-11-27 21:19           ` Johan Hedberg
  0 siblings, 0 replies; 9+ messages in thread
From: Johan Hedberg @ 2018-11-27 21:19 UTC (permalink / raw)
  To: Zumeng Chen; +Cc: linux-bluetooth

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-11-27 21:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-08  5:47 [PATCH 1/1] Bluetooth: make the balance of judgement condition to fix a false report Zumeng Chen
2018-11-14  7:54 ` Marcel Holtmann
2018-11-15  1:31   ` [PATCH 1/1 v2] " Zumeng Chen
2018-11-26 11:23     ` Luiz Augusto von Dentz
2018-11-27  9:24     ` Johan Hedberg
2018-11-27  9:34       ` Johan Hedberg
2018-11-27 12:22         ` Zumeng Chen
2018-11-27 21:19           ` Johan Hedberg
2018-11-27 12:13       ` Zumeng Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).