Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Apr 23, 2019 at 1:08 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andrey,
>
> > Due to:
> >
> > - current implementation of l2cap_config_rsp() dropping BT
> >   connection if sender of configuration response replied with unknown
> >   option failure (Result=0x0003/L2CAP_CONF_UNKNOWN)
> >
> > - current implementation of l2cap_build_conf_req() adding
> >   L2CAP_CONF_RFC(0x04) option to initial configure request sent by
> >   the Linux host.
> >
> > devices that do no recongninze L2CAP_CONF_RFC, such as Xbox One S
> > controllers, will get stuck in endless connect -> configure ->
> > disconnect loop, never connect and be generaly unusable.
> >
> > To avoid this problem add code to do the following:
> >
> > 1. Store a mask of supported conf option types per connection
> >
> > 2. Parse the body of response L2CAP_CONF_UNKNOWN and adjust
> >    connection's supported conf option types mask
> >
> > 3. Retry configuration step the same way it's done for
> >    L2CAP_CONF_UNACCEPT
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> > Cc: Florian Dollinger <dollinger.florian@gmx.de>
> > Cc: Marcel Holtmann <marcel@holtmann.org>
> > Cc: Johan Hedberg <johan.hedberg@gmail.com>
> > Cc: linux-bluetooth@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >
> > Everyone:
> >
> > I marked this as an RFC, since I don't have a lot of experience with
> > Bluetooth subsystem and don't have hight degree of confidence about
> > choices made in this patch. I do, however, thins is is good enough to
> > start a discussion about the problem.
> >
> > Thanks,
> > Andrey Smirnov
>
> so it seems that the remote side claims to support Streaming Mode and that is why we are trying to set it up.
>
> > ACL Data RX: Handle 12 flags 0x02 dlen 16
>       L2CAP: Information Response (0x0b) ident 1 len 8
>         Type: Extended features supported (0x0002)
>         Result: Success (0x0000)
>         Features: 0x00000010
>           Streaming Mode
>
> And that is why we do this.
>
> < ACL Data TX: Handle 12 flags 0x00 dlen 23
>       L2CAP: Configure Request (0x04) ident 2 len 15
>         Destination CID: 64
>         Flags: 0x0000
>         Option: Retransmission and Flow Control (0x04) [mandatory]
>           Mode: Basic (0x00)
>           TX window size: 0
>           Max transmit: 0
>           Retransmission timeout: 0
>           Monitor timeout: 0
>           Maximum PDU size: 0
>
> > ACL Data RX: Handle 12 flags 0x02 dlen 15
>       L2CAP: Configure Response (0x05) ident 2 len 7
>         Source CID: 64
>         Flags: 0x0000
>         Result: Failure - unknown options (0x0003)
>         04
>
> So btmon needs a patch to decide the failed option octet here. We really want do provide a human description of the failed option.
>

I'll see if that's an easy thing to add. Can't promise anything though.

> >
> > include/net/bluetooth/l2cap.h |  1 +
> > net/bluetooth/l2cap_core.c    | 58 ++++++++++++++++++++++++++++++-----
> > 2 files changed, 51 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 093aedebdf0c..6898bba5d9a8 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -632,6 +632,7 @@ struct l2cap_conn {
> >       unsigned int            mtu;
> >
> >       __u32                   feat_mask;
> > +     __u32                   known_options;
> >       __u8                    remote_fixed_chan;
> >       __u8                    local_fixed_chan;
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index f17e393b43b4..49be98b6de72 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -3243,8 +3243,10 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >               rfc.monitor_timeout = 0;
> >               rfc.max_pdu_size    = 0;
> >
> > -             l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > -                                (unsigned long) &rfc, endptr - ptr);
> > +             if (chan->conn->known_options & BIT(L2CAP_CONF_RFC)) {
> > +                     l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > +                                        (unsigned long)&rfc, endptr - ptr);
> > +             }
> >               break;
> >
> >       case L2CAP_MODE_ERTM:
> > @@ -3263,8 +3265,10 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >               rfc.txwin_size = min_t(u16, chan->tx_win,
> >                                      L2CAP_DEFAULT_TX_WINDOW);
> >
> > -             l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > -                                (unsigned long) &rfc, endptr - ptr);
> > +             if (chan->conn->known_options & BIT(L2CAP_CONF_RFC)) {
> > +                     l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > +                                        (unsigned long)&rfc, endptr - ptr);
> > +             }
> >
> >               if (test_bit(FLAG_EFS_ENABLE, &chan->flags))
> >                       l2cap_add_opt_efs(&ptr, chan, endptr - ptr);
> > @@ -3295,8 +3299,10 @@ static int l2cap_build_conf_req(struct l2cap_chan *chan, void *data, size_t data
> >                            L2CAP_FCS_SIZE);
> >               rfc.max_pdu_size = cpu_to_le16(size);
> >
> > -             l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > -                                (unsigned long) &rfc, endptr - ptr);
> > +             if (chan->conn->known_options & BIT(L2CAP_CONF_RFC)) {
> > +                     l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
> > +                                        (unsigned long)&rfc, endptr - ptr);
> > +             }
> >
> >               if (test_bit(FLAG_EFS_ENABLE, &chan->flags))
> >                       l2cap_add_opt_efs(&ptr, chan, endptr - ptr);
> > @@ -3550,11 +3556,47 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len,
> >       void *endptr = data + size;
> >       int type, olen;
> >       unsigned long val;
> > +     const bool unknown_options = *result == L2CAP_CONF_UNKNOWN;
> >       struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
> >       struct l2cap_conf_efs efs;
> >
> >       BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
> >
> > +     /* throw out any old stored conf requests */
> > +     *result = L2CAP_CONF_SUCCESS;
> > +
> > +     if (unknown_options) {
> > +             const u8 *option_type = rsp;
> > +
> > +             if (!len) {
> > +                     /* If no list of unknown option types is
> > +                      * provided there's nothing for us to do
> > +                      */
> > +                     return -ECONNREFUSED;
> > +             }
> > +
> > +             while (len--) {
> > +                     BT_DBG("chan %p, unknown option type: %u", chan,
> > +                            *option_type);
> > +                     /* "...Hints shall not be included in the
> > +                      * Response and shall not be the sole cause
> > +                      * for rejecting the Request.."
> > +                      */
> > +                     if (*option_type & L2CAP_CONF_HINT)
> > +                             return -ECONNREFUSED;
> > +                     /* Make sure option type is one of the types
> > +                      * supported/used in configure requests
> > +                      */
> > +                     if (*option_type < L2CAP_CONF_MTU ||
> > +                         *option_type > L2CAP_CONF_EWS)
> > +                             return -ECONNREFUSED;
> > +
> > +                     chan->conn->known_options &= ~BIT(*option_type++);
> > +             }
> > +
> > +             return l2cap_build_conf_req(chan, data, size);
> > +     }
> > +
> >       while (len >= L2CAP_CONF_OPT_SIZE) {
> >               len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
> >               if (len < 0)
> > @@ -4240,6 +4282,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
> >               }
> >               goto done;
> >
> > +     case L2CAP_CONF_UNKNOWN:
> >       case L2CAP_CONF_UNACCEPT:
> >               if (chan->num_conf_rsp <= L2CAP_CONF_MAX_CONF_RSP) {
> >                       char req[64];
> > @@ -4249,8 +4292,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
> >                               goto done;
> >                       }
> >
> > -                     /* throw out any old stored conf requests */
> > -                     result = L2CAP_CONF_SUCCESS;
> >                       len = l2cap_parse_conf_rsp(chan, rsp->data, len,
> >                                                  req, sizeof(req), &result);
> >                       if (len < 0) {
>
> So I really wonder if we want to combine CONF_UNKNOWN and CONF_UNACCEPT actually here. It might be better to do them as separate handlers.

I just wanted to minimize all of the surrounding boilerplate code
duplication. Will change v2 to have a separate handler.

>
> Frankly it might be enough if the option code 0x04 is marked as not supported, then just clear
>
>         conn->feat_mask &= ~(L2CAP_FEAT_ERTM | L2CAP_FEAT_STREAMING);
>
> There is really no point for us known about all unsupported / unknown options. Unless we understand them, then don’t bother. Just keep it simple.
>

OK, sure, I think that should work. I'll give it a try and report back in v2.

Thanks,
Andrey Smirnov