PTR_ERR() returns a long type value, but btusb_setup_intel() and
btusb_setup_intel_patching() should return an int type value.
This bug makes the judgement "if (ret < 0)" not working on x86_64
architecture systems, leading to failure as below, even panic.
[ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
[ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
[ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
[ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
[ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
[ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
[ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
[ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
[ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
[ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
[ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
[ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
[ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
[ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
[ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
[ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
[ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
[ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
For not affecting other modules, I choose to modify the return values
but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
return types. This is harmless, because the return values were only
used to comparing number 0.
Signed-off-by: Adam Lee <[email protected]>
---
drivers/bluetooth/btusb.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a7e5f8..0fb2799 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev *hdev,
if (IS_ERR(skb)) {
BT_ERR("%s sending Intel patch command (0x%4.4x) failed (%ld)",
hdev->name, cmd->opcode, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
/* It ensures that the returned event matches the event data read from
@@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s sending initial HCI reset command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s reading Intel fw version command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
if (skb->len != sizeof(*ver)) {
@@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
release_firmware(fw);
- return -PTR_ERR(skb);
+ return -EFAULT;
}
if (skb->data[0]) {
@@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1289,7 +1289,7 @@ exit_mfg_disable:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
--
1.8.3.2
Hi Adam,
* Adam Lee <[email protected]> [2013-07-10 10:02:12 +0800]:
> PTR_ERR() returns a signed long type value which is limited by IS_ERR(),
> it must be a negative number whose range is [-MAX_ERRNO, 0).
>
> The bug here returns negative numbers as error codes, then check it by
> "if (ret < 0)", but -PTR_ERR() is actually positive. The wrong use here
> leads to failure as below, even panic.
>
> [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
> [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
> [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
> [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
> [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
> [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
> [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
>
> Signed-off-by: Adam Lee <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Patch has been applied to bluetooth.git. Thanks.
Gustavo
PTR_ERR() returns a signed long type value which is limited by IS_ERR(),
it must be a negative number whose range is [-MAX_ERRNO, 0).
The bug here returns negative numbers as error codes, then check it by
"if (ret < 0)", but -PTR_ERR() is actually positive. The wrong use here
leads to failure as below, even panic.
[ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
[ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
[ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
[ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
[ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
[ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
[ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
[ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
[ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
[ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
[ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
[ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
[ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
[ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
[ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
[ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
[ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
[ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
Signed-off-by: Adam Lee <[email protected]>
---
drivers/bluetooth/btusb.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a7e5f8..23df968 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev *hdev,
if (IS_ERR(skb)) {
BT_ERR("%s sending Intel patch command (0x%4.4x) failed (%ld)",
hdev->name, cmd->opcode, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
/* It ensures that the returned event matches the event data read from
@@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s sending initial HCI reset command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
kfree_skb(skb);
@@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s reading Intel fw version command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
if (skb->len != sizeof(*ver)) {
@@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
release_firmware(fw);
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
if (skb->data[0]) {
@@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
kfree_skb(skb);
@@ -1289,7 +1289,7 @@ exit_mfg_disable:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
kfree_skb(skb);
@@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return PTR_ERR(skb);
}
kfree_skb(skb);
--
1.8.3.2
Hi Adam,
* Adam Lee <[email protected]> [2013-07-09 10:55:01 +0800]:
> On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote:
> > Hi Adam,
> >
> > > PTR_ERR() returns a long type value, but btusb_setup_intel() and
> > > btusb_setup_intel_patching() should return an int type value.
> > >
> > > This bug makes the judgement "if (ret < 0)" not working on x86_64
> > > architecture systems, leading to failure as below, even panic.
> > > ...
> > > For not affecting other modules, I choose to modify the return values
> > > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
> > > return types. This is harmless, because the return values were only
> > > used to comparing number 0.
> >
> > there are tons of examples in various subsystems and drivers where we
> > return PTR_ERR from a function calls returning int.
> >
> > So I wonder what is actually going wrong here. If this is x86_64
> > specific problem with PTR_ERR vs int, then we should have this problem
> > everywhere in the kernel.
>
> Hi, Marcel
>
> I see you point, the difference between here and other subsystems are:
>
> 1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I
> checked.
Please sending a patch fixing this. We got this right in other parts of the
bluetooth subsystems but somehow we failed to check this when this code came
in. And then another updating the checks if needed.
Gustavo
PTR_ERR() returns a signed long type value which is limited by IS_ERR(),
it must be a negative number and bigger than -MAX_ERRNO(-4095).
The bug here, btusb_setup_intel_patching()'s return value might come
from -PTR_ERR() which must be a positive number, we can't judge if it's
an error by "if (ret < 0)", the wrong use here leads to failure as
below, even panic.
Error log:
[ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
[ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
[ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
[ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
[ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
[ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
[ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
[ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
[ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
[ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
[ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
[ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
[ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
[ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
[ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
[ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
[ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
[ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
Signed-off-by: Adam Lee <[email protected]>
---
drivers/bluetooth/btusb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a7e5f8..05a38a2 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1256,7 +1256,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
ret = btusb_setup_intel_patching(hdev, fw, &fw_ptr,
&disable_patch);
- if (ret < 0)
+ if (!ret)
goto exit_mfg_deactivate;
}
--
1.8.3.2
On Tue, Jul 09, 2013 at 10:55:01AM +0800, Adam Lee wrote:
> On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote:
> > Hi Adam,
> >
> > > PTR_ERR() returns a long type value, but btusb_setup_intel() and
> > > btusb_setup_intel_patching() should return an int type value.
> > >
> > > This bug makes the judgement "if (ret < 0)" not working on x86_64
> > > architecture systems, leading to failure as below, even panic.
> > > ...
> > > For not affecting other modules, I choose to modify the return values
> > > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
> > > return types. This is harmless, because the return values were only
> > > used to comparing number 0.
> >
> > there are tons of examples in various subsystems and drivers where we
> > return PTR_ERR from a function calls returning int.
> >
> > So I wonder what is actually going wrong here. If this is x86_64
> > specific problem with PTR_ERR vs int, then we should have this problem
> > everywhere in the kernel.
>
> Hi, Marcel
>
> I see you point, the difference between here and other subsystems are:
>
> 1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I
> checked.
> 2, the judgement is "if (ret < 0)" here but other places are "if (ret)".
>
> I'm not saying other subsystems are 100% right, but here, returning
> -PTR_ERR() and checking "if (ret < 0)" make the judgement broken much
> much more easily.
>
> I attached a testing C file, run it on x86_64, you will see the bug.
>
> PS, about other subsystems, I also think returning PTR_ERR() from a
> function calls returning int considered harmful sometimes, will talk
> about that in other thread.
Hi, all
After diving into the err.h, I realized this patch contains some
modifications which are actually not necessary. Will submit a v2
version and explain.
PS, other subsystems are using it right, this not.
--
Regards,
Adam Lee
Hardware Enablement
On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote:
> Hi Adam,
>
> > PTR_ERR() returns a long type value, but btusb_setup_intel() and
> > btusb_setup_intel_patching() should return an int type value.
> >
> > This bug makes the judgement "if (ret < 0)" not working on x86_64
> > architecture systems, leading to failure as below, even panic.
> > ...
> > For not affecting other modules, I choose to modify the return values
> > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
> > return types. This is harmless, because the return values were only
> > used to comparing number 0.
>
> there are tons of examples in various subsystems and drivers where we
> return PTR_ERR from a function calls returning int.
>
> So I wonder what is actually going wrong here. If this is x86_64
> specific problem with PTR_ERR vs int, then we should have this problem
> everywhere in the kernel.
Hi, Marcel
I see you point, the difference between here and other subsystems are:
1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I
checked.
2, the judgement is "if (ret < 0)" here but other places are "if (ret)".
I'm not saying other subsystems are 100% right, but here, returning
-PTR_ERR() and checking "if (ret < 0)" make the judgement broken much
much more easily.
I attached a testing C file, run it on x86_64, you will see the bug.
PS, about other subsystems, I also think returning PTR_ERR() from a
function calls returning int considered harmful sometimes, will talk
about that in other thread.
Great thanks.
--
Regards,
Adam Lee
Hardware Enablement
Hi Adam,
> PTR_ERR() returns a long type value, but btusb_setup_intel() and
> btusb_setup_intel_patching() should return an int type value.
>
> This bug makes the judgement "if (ret < 0)" not working on x86_64
> architecture systems, leading to failure as below, even panic.
>
> [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
> [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
> [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
> [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
> [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
> [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
> [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
> [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
>
> For not affecting other modules, I choose to modify the return values
> but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
> return types. This is harmless, because the return values were only
> used to comparing number 0.
there are tons of examples in various subsystems and drivers where we return PTR_ERR from a function calls returning int.
So I wonder what is actually going wrong here. If this is x86_64 specific problem with PTR_ERR vs int, then we should have this problem everywhere in the kernel.
Regards
Marcel
On Fri, Jul 05, 2013 at 10:59:47AM +0800, Adam Lee wrote:
> On Fri, Jul 05, 2013 at 10:37:07AM +0800, Yang Bai wrote:
> > The return value of?btusb_setup_intel is compared with 0. Code as:
> >
> > drivers/bluetooth/btusb.c:
> > static int btusb_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > if (id->driver_info & BTUSB_INTEL)
> > hdev->setup = btusb_setup_intel;
> >
> > net/bluetooth/hci_core.c:
> > int hci_dev_open(__u16 dev)
> > if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
> > ret = hdev->setup(hdev);
> >
> > if (!ret) {
>
> Yes, for btusb_setup_intel(), the return value is compared with number
> "0", doesn't break the judgement.
>
> But it still overflows stack without this fix.
And what if the lower 32bits of PTR_ERR() are all "0"? :D
> > On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <[email protected]> wrote:
> >
> > PTR_ERR() returns a long type value, but btusb_setup_intel() and
> > btusb_setup_intel_patching() should return an int type value.
> >
> > This bug makes the judgement "if (ret < 0)" not working on x86_64
> > architecture systems, leading to failure as below, even panic.
--
Regards,
Adam Lee
Hardware Enablement
On Fri, Jul 05, 2013 at 10:37:07AM +0800, Yang Bai wrote:
> The return value of?btusb_setup_intel is compared with 0. Code as:
>
> drivers/bluetooth/btusb.c:
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> if (id->driver_info & BTUSB_INTEL)
> hdev->setup = btusb_setup_intel;
>
> net/bluetooth/hci_core.c:
> int hci_dev_open(__u16 dev)
> if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
> ret = hdev->setup(hdev);
>
> if (!ret) {
Yes, for btusb_setup_intel(), the return value is compared with number
"0", doesn't break the judgement.
But it still overflows stack without this fix.
> On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <[email protected]> wrote:
>
> PTR_ERR() returns a long type value, but btusb_setup_intel() and
> btusb_setup_intel_patching() should return an int type value.
>
> This bug makes the judgement "if (ret < 0)" not working on x86_64
> architecture systems, leading to failure as below, even panic.
--
Regards,
Adam Lee
Hardware Enablement
Resend without HTML format.
The return value of btusb_setup_intel is compared with 0. Code as:
drivers/bluetooth/btusb.c:
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id) {
if (id->driver_info & BTUSB_INTEL)
hdev->setup =3D btusb_setup_intel;
}
net/bluetooth/hci_core.c:
int hci_dev_open(__u16 dev) {
if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
ret =3D hdev->setup(hdev);
if (!ret) {
...
}
}
Thanks,
Yang
On Fri, Jul 5, 2013 at 10:37 AM, Yang Bai <[email protected]> wrote:
> The return value of btusb_setup_intel is compared with 0. Code as:
>
> drivers/bluetooth/btusb.c:
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> if (id->driver_info & BTUSB_INTEL)
> hdev->setup =3D btusb_setup_intel;
>
> net/bluetooth/hci_core.c:
> int hci_dev_open(__u16 dev)
> if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
> ret =3D hdev->setup(hdev);
>
> if (!ret) {
>
>
> Thanks,
> Yang
>
>
>
> On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <[email protected]> wrote:
>>
>> PTR_ERR() returns a long type value, but btusb_setup_intel() and
>> btusb_setup_intel_patching() should return an int type value.
>>
>> This bug makes the judgement "if (ret < 0)" not working on x86_64
>> architecture systems, leading to failure as below, even panic.
>>
>> [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
>> [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) fail=
ed
>> (-110)
>> [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
>> [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) fail=
ed
>> (-110)
>> [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
>> [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) fail=
ed
>> (-110)
>> [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
>> [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) fail=
ed
>> (-110)
>> [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
>> [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed
>> (-110)
>> [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp
>> 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
>>
>> For not affecting other modules, I choose to modify the return values
>> but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
>> return types. This is harmless, because the return values were only
>> used to comparing number 0.
>>
>> Signed-off-by: Adam Lee <[email protected]>
>> ---
>> drivers/bluetooth/btusb.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 7a7e5f8..0fb2799 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_d=
ev
>> *hdev,
>> if (IS_ERR(skb)) {
>> BT_ERR("%s sending Intel patch command (0x%4.4x) failed
>> (%ld)",
>> hdev->name, cmd->opcode, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>>
>> /* It ensures that the returned event matches the event data rea=
d
>> from
>> @@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
>> if (IS_ERR(skb)) {
>> BT_ERR("%s sending initial HCI reset command failed
>> (%ld)",
>> hdev->name, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>> kfree_skb(skb);
>>
>> @@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
>> if (IS_ERR(skb)) {
>> BT_ERR("%s reading Intel fw version command failed (%ld)=
",
>> hdev->name, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>>
>> if (skb->len !=3D sizeof(*ver)) {
>> @@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
>> BT_ERR("%s entering Intel manufacturer mode failed (%ld)=
",
>> hdev->name, PTR_ERR(skb));
>> release_firmware(fw);
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>>
>> if (skb->data[0]) {
>> @@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
>> if (IS_ERR(skb)) {
>> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)"=
,
>> hdev->name, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>> kfree_skb(skb);
>>
>> @@ -1289,7 +1289,7 @@ exit_mfg_disable:
>> if (IS_ERR(skb)) {
>> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)"=
,
>> hdev->name, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>> kfree_skb(skb);
>>
>> @@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
>> if (IS_ERR(skb)) {
>> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)"=
,
>> hdev->name, PTR_ERR(skb));
>> - return -PTR_ERR(skb);
>> + return -EFAULT;
>> }
>> kfree_skb(skb);
>>
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" =
in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
>
> --
> """
> Keep It Simple,Stupid.
> """
>
> Chinese Name: =E7=99=BD=E6=9D=A8
> Nick Name: Hamo
> Homepage: http://hamobai.com/
> GPG KEY ID: 0xA4691A33
> Key fingerprint =3D 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33
--=20
"""
Keep It Simple,Stupid.
"""
Chinese Name: =E7=99=BD=E6=9D=A8
Nick Name: Hamo
Homepage: http://hamobai.com/
GPG KEY ID: 0xA4691A33
Key fingerprint =3D 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33
The return value of btusb_setup_intel is compared with 0. Code as:
drivers/bluetooth/btusb.c:
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
if (id->driver_info & BTUSB_INTEL)
hdev->setup = btusb_setup_intel;
net/bluetooth/hci_core.c:
int hci_dev_open(__u16 dev)
if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
ret = hdev->setup(hdev);
if (!ret) {
Thanks,
Yang
On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <[email protected]> wrote:
> PTR_ERR() returns a long type value, but btusb_setup_intel() and
> btusb_setup_intel_patching() should return an int type value.
>
> This bug makes the judgement "if (ret < 0)" not working on x86_64
> architecture systems, leading to failure as below, even panic.
>
> [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
> [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed
> (-110)
> [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
> [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed
> (-110)
> [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed
> (-110)
> [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
> [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed
> (-110)
> [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
> [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed
> (-110)
> [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp
> 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
>
> For not affecting other modules, I choose to modify the return values
> but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
> return types. This is harmless, because the return values were only
> used to comparing number 0.
>
> Signed-off-by: Adam Lee <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 7a7e5f8..0fb2799 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev
> *hdev,
> if (IS_ERR(skb)) {
> BT_ERR("%s sending Intel patch command (0x%4.4x) failed
> (%ld)",
> hdev->name, cmd->opcode, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
>
> /* It ensures that the returned event matches the event data read
> from
> @@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> if (IS_ERR(skb)) {
> BT_ERR("%s sending initial HCI reset command failed (%ld)",
> hdev->name, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
> kfree_skb(skb);
>
> @@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> if (IS_ERR(skb)) {
> BT_ERR("%s reading Intel fw version command failed (%ld)",
> hdev->name, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
>
> if (skb->len != sizeof(*ver)) {
> @@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
> hdev->name, PTR_ERR(skb));
> release_firmware(fw);
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
>
> if (skb->data[0]) {
> @@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
> if (IS_ERR(skb)) {
> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
> hdev->name, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
> kfree_skb(skb);
>
> @@ -1289,7 +1289,7 @@ exit_mfg_disable:
> if (IS_ERR(skb)) {
> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
> hdev->name, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
> kfree_skb(skb);
>
> @@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
> if (IS_ERR(skb)) {
> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
> hdev->name, PTR_ERR(skb));
> - return -PTR_ERR(skb);
> + return -EFAULT;
> }
> kfree_skb(skb);
>
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
"""
Keep It Simple,Stupid.
"""
Chinese Name: 白杨
Nick Name: Hamo
Homepage: http://hamobai.com/
GPG KEY ID: 0xA4691A33
Key fingerprint = 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33