btio: Fix registering Object Push, File Transfer and other L2CAP profiles

PATCH In review Pali Rohár 3 participants 5 messages Jun 4, 2020 – Jun 12, 2020 Bluetooth drivers

lore

Share & tools

lore

Pali Rohár

author

2020-06-04 23:16 UTC #0 lore raw

PATCH 1/1 btio: Fix registering Object Push, File Transfer and other L2CAP profiles

When bluetoothd daemon is starting, it prints following error messages:

bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Notification: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Phone Book Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for File Transfer: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Object Push: setsockopt(L2CAP_OPTIONS): Invalid argument (22)

Result is that Object Push and File Transfer profiles do not work at all.

Debugging set_l2opts() function showed me that src/profile.c calls it with L2CAP mode 0x01 (L2CAP_MODE_RETRANS). But kernel bluetooth code in function l2cap_sock_setsockopt_old() for L2CAP_OPTIONS option disallows to set L2CAP method to 0x01 (L2CAP_MODE_RETRANS) and just returns -EINVAL (22).

These bluetooth profiles have in src/profile.c file defined L2CAP mode to BT_IO_MODE_ERTM and not to RETRANS. So it means that BT_IO_MODE_ERTM value defined in 'enum BtIOMode' must be incorrect.

Digging into git history, it appears that 'enum BtIOMode' was broken in commit f2418bf97d ("btio: Add mode to for Enhanced Credit Mode") which basically broke all those Object Push, File Transfer, Phone Book Access, Message Access and Message Notification L2CAP profiles. That commit removed some values from 'enum BtIOMode' and therefore broke all bluetooth code which uses 'enum BtIOMode' for communication with kernel.

This patch fixes 'enum BtIOMode' by reverting back BT_IO_MODE_RETRANS and BT_IO_MODE_FLOWCTL modes, so BT_IO_MODE_ERTM has again correct value 0x03.

After applying this patch, Object Push and File Transfer profiles work again.

Fixes f2418bf97d ("btio: Add mode to for Enhanced Credit Mode")

---

Hello Luiz, could you please review this patch? As that problematic commit f2418bf97d was introduced by you.

I'm curious why nobody else reported this issue about broken Object Push and File Transfer profile as it should print those error messages... Or maybe error message is visible only in debug build and nobody is using Bluetooth File Transfer anymore?

---
 btio/btio.h | 2 ++
 1 file changed, 2 insertions(+)

btio/btio.h +2 -0

index 23e0ef72b..0d183e3ce 100644
@@ -68,6 +68,8 @@ typedef enum {
 
 typedef enum {
 	BT_IO_MODE_BASIC = 0,
+	BT_IO_MODE_RETRANS,
+	BT_IO_MODE_FLOWCTL,
 	BT_IO_MODE_ERTM,
 	BT_IO_MODE_STREAMING,
 	BT_IO_MODE_LE_FLOWCTL,

Signature · 1 line

2.20.1

Luiz Augusto von Dentz

maintainer

2020-06-05 00:12 UTC #1 lore raw

Hi Pali,

Quoted 36 lines · Pali Rohár

When bluetoothd daemon is starting, it prints following error messages:
bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Notification: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Phone Book Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for File Transfer: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Object Push: setsockopt(L2CAP_OPTIONS): Invalid argument (22)
Result is that Object Push and File Transfer profiles do not work at all.
Debugging set_l2opts() function showed me that src/profile.c calls it with L2CAP mode 0x01 (L2CAP_MODE_RETRANS). But kernel bluetooth code in function l2cap_sock_setsockopt_old() for L2CAP_OPTIONS option disallows to set L2CAP method to 0x01 (L2CAP_MODE_RETRANS) and just returns -EINVAL (22).
These bluetooth profiles have in src/profile.c file defined L2CAP mode to BT_IO_MODE_ERTM and not to RETRANS. So it means that BT_IO_MODE_ERTM value defined in 'enum BtIOMode' must be incorrect.
Digging into git history, it appears that 'enum BtIOMode' was broken in commit f2418bf97d ("btio: Add mode to for Enhanced Credit Mode") which basically broke all those Object Push, File Transfer, Phone Book Access, Message Access and Message Notification L2CAP profiles. That commit removed some values from 'enum BtIOMode' and therefore broke all bluetooth code which uses 'enum BtIOMode' for communication with kernel.
This patch fixes 'enum BtIOMode' by reverting back BT_IO_MODE_RETRANS and BT_IO_MODE_FLOWCTL modes, so BT_IO_MODE_ERTM has again correct value 0x03.
After applying this patch, Object Push and File Transfer profiles work again.
Fixes f2418bf97d ("btio: Add mode to for Enhanced Credit Mode")
---

Quoted 7 lines

Hello Luiz, could you please review this patch? As that problematic commit f2418bf97d was introduced by you.
I'm curious why nobody else reported this issue about broken Object Push and File Transfer profile as it should print those error messages... Or maybe error message is visible only in debug build and nobody is using Bluetooth File Transfer anymore?

Looks like you are right but, see bellow.

Quoted 18 lines

---
 btio/btio.h | 2 ++
 1 file changed, 2 insertions(+)

btio/btio.h +2 -1

index 23e0ef72b..0d183e3ce 100644
@@ -68,6 +68,8 @@ typedef enum {
 
 typedef enum {
        BT_IO_MODE_BASIC = 0,
+       BT_IO_MODE_RETRANS,
+       BT_IO_MODE_FLOWCTL,
        BT_IO_MODE_ERTM,
        BT_IO_MODE_STREAMING,
        BT_IO_MODE_LE_FLOWCTL,
--

Quoted 1 line

2.20.1

These modes were never supported by the kernel thus why Ive dropped them, so might want to translate them to the old L2CAP modes when set_l2opts is called.

Signature · 1 line

Luiz Augusto von Dentz

Pali Rohár

author

2020-06-12 13:12 UTC #2 lore raw

Quoted 59 lines · On Friday 05 June 2020 00:11:54 Luiz Augusto von Dentz wrote:

Hi Pali,
Quoted 36 lines · Pali Rohár
When bluetoothd daemon is starting, it prints following error messages:
bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Notification: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Phone Book Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for File Transfer: setsockopt(L2CAP_OPTIONS): Invalid argument (22) bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Object Push: setsockopt(L2CAP_OPTIONS): Invalid argument (22)
Result is that Object Push and File Transfer profiles do not work at all.
Debugging set_l2opts() function showed me that src/profile.c calls it with L2CAP mode 0x01 (L2CAP_MODE_RETRANS). But kernel bluetooth code in function l2cap_sock_setsockopt_old() for L2CAP_OPTIONS option disallows to set L2CAP method to 0x01 (L2CAP_MODE_RETRANS) and just returns -EINVAL (22).
These bluetooth profiles have in src/profile.c file defined L2CAP mode to BT_IO_MODE_ERTM and not to RETRANS. So it means that BT_IO_MODE_ERTM value defined in 'enum BtIOMode' must be incorrect.
Digging into git history, it appears that 'enum BtIOMode' was broken in commit f2418bf97d ("btio: Add mode to for Enhanced Credit Mode") which basically broke all those Object Push, File Transfer, Phone Book Access, Message Access and Message Notification L2CAP profiles. That commit removed some values from 'enum BtIOMode' and therefore broke all bluetooth code which uses 'enum BtIOMode' for communication with kernel.
This patch fixes 'enum BtIOMode' by reverting back BT_IO_MODE_RETRANS and BT_IO_MODE_FLOWCTL modes, so BT_IO_MODE_ERTM has again correct value 0x03.
After applying this patch, Object Push and File Transfer profiles work again.
Fixes f2418bf97d ("btio: Add mode to for Enhanced Credit Mode")
---
Quoted 7 lines
Hello Luiz, could you please review this patch? As that problematic commit f2418bf97d was introduced by you.
I'm curious why nobody else reported this issue about broken Object Push and File Transfer profile as it should print those error messages... Or maybe error message is visible only in debug build and nobody is using Bluetooth File Transfer anymore?
Looks like you are right but, see bellow.
Quoted 10 lines
---
 btio/btio.h | 2 ++
 1 file changed, 2 insertions(+)
btio/btio.h +0 -0
index 23e0ef72b..0d183e3ce 100644
@@ -68,6 +68,8 @@ typedef enum {
 

Quoted 13 lines

Quoted 9 lines
btio/btio.h +2 -1
 typedef enum {
        BT_IO_MODE_BASIC = 0,
+       BT_IO_MODE_RETRANS,
+       BT_IO_MODE_FLOWCTL,
        BT_IO_MODE_ERTM,
        BT_IO_MODE_STREAMING,
        BT_IO_MODE_LE_FLOWCTL,
--
2.20.1
These modes were never supported by the kernel thus why Ive dropped them, so might want to translate them to the old L2CAP modes when set_l2opts is called.

Hello Luiz! In commit 81629d982c672e4b3288c4499f9912f60f7040e9 ("btio:
Fix not translation mode to L2CAP mode") you have introduced following code:

+ l2o.mode = mode_l2mode(mode);
+ if (l2o.mode == UINT8_MAX) {
+ ERROR_FAILED(err, "Unsupported mode", errno);
+ return FALSE;
+ }

But function mode_l2mode() does not set errno when it fails. Therefore it reports bogus error number (probably zero).

So probably you want to use EINVAL (or some other constant):

+ ERROR_FAILED(err, "Unsupported mode", EINVAL);

[email protected]

2020-06-05 00:12 UTC #3 lore raw

In reply to Pali Rohár

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list. While we are preparing for reviewing the patches, we found the following issue/warning.

Test Result:
checkpatch Failed

Outputs:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#9:
bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Notification: setsockopt(L2CAP_OPTIONS): Invalid argument (22)

ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f2418bf97dec ("btio: Add mode to for Enhanced Credit Mode")' #27:
commit f2418bf97d ("btio: Add mode to for Enhanced Credit Mode") which

- total: 1 errors, 1 warnings, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

---
Regards,
Linux Bluetooth

[email protected]

2020-06-05 00:12 UTC #4 lore raw

In reply to Pali Rohár

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list. While we are preparing for reviewing the patches, we found the following issue/warning.

Test Result:
checkgitlint Failed

Outputs:
1: T1 Title exceeds max length (73>72): "btio: Fix registering Object Push, File Transfer and other L2CAP profiles"
5: B1 Line exceeds max length (147>80): "bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Notification: setsockopt(L2CAP_OPTIONS): Invalid argument (22)" 6: B1 Line exceeds max length (141>80): "bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Message Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22)" 7: B1 Line exceeds max length (144>80): "bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Phone Book Access: setsockopt(L2CAP_OPTIONS): Invalid argument (22)" 8: B1 Line exceeds max length (140>80): "bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for File Transfer: setsockopt(L2CAP_OPTIONS): Invalid argument (22)" 9: B1 Line exceeds max length (138>80): "bluetoothd[19117]: src/profile.c:ext_start_servers() L2CAP server failed for Object Push: setsockopt(L2CAP_OPTIONS): Invalid argument (22)"

---
Regards,
Linux Bluetooth