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

["Pali Rohár" <pali@kernel.org>]

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);