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

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

[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")
---

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(+)

diff --git a/btio/btio.h b/btio/btio.h
index 23e0ef72b..0d183e3ce 100644
--- a/btio/btio.h
+++ b/btio/btio.h
@@ -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,
-- 
2.20.1

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]


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

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]


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

Re: [PATCH] btio: Fix registering Object Push, File Transfer and other L2CAP profiles

[Luiz Augusto von Dentz]

Hi Pali,

On Thu, Jun 4, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
>
> 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?

Looks like you are right but, see bellow.

> ---
>  btio/btio.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/btio/btio.h b/btio/btio.h
> index 23e0ef72b..0d183e3ce 100644
> --- a/btio/btio.h
> +++ b/btio/btio.h
> @@ -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,
> --
> 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.


-- 
Luiz Augusto von Dentz

Re: [PATCH] btio: Fix registering Object Push, File Transfer and other L2CAP profiles

[Pali Rohár]

On Friday 05 June 2020 00:11:54 Luiz Augusto von Dentz wrote:
> Hi Pali,
> 
> On Thu, Jun 4, 2020 at 4:18 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > 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?
> 
> Looks like you are right but, see bellow.
> 
> > ---
> >  btio/btio.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/btio/btio.h b/btio/btio.h
> > index 23e0ef72b..0d183e3ce 100644
> > --- a/btio/btio.h
> > +++ b/btio/btio.h
> > @@ -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,
> > --
> > 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);