Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20190906023601.4378-1-andrew.smirnov@gmail.com>
In-Reply-To: <20190906023601.4378-1-andrew.smirnov@gmail.com>
From:   Andrey Smirnov <andrew.smirnov@gmail.com>
Date:   Mon, 13 Apr 2020 20:18:07 -0700
Message-ID: <CAHQ1cqEqM5=Gm1jxEX=h02KW_-OLUn0W3R7T6o5bFW9+6ZPiOQ@mail.gmail.com>
Subject: Re: [RESEND PATCH v2] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
To:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        linux-bluetooth@vger.kernel.org
Cc:     "Pierre-Loup A . Griffais" <pgriffais@valvesoftware.com>,
        Florian Dollinger <dollinger.florian@gmx.de>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 5, 2019 at 7:36 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> 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. Parse the body of response L2CAP_CONF_UNKNOWN and, in case of
>     unsupported option being RFC, clear L2CAP_FEAT_ERTM and
>     L2CAP_FEAT_STREAMING from connection's feature mask (in order to
>     prevent RFC option from being added going forward)
>
>  2. 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
> ---
>
> Changes since [v1]:
>
>    - Patch simplified to simply clear L2CAP_FEAT_ERTM |
>      L2CAP_FEAT_STREAMING from feat_mask when device flags RFC options
>      as unknown
>
> [v1] lore.kernel.org/r/20190208025828.30901-1-andrew.smirnov@gmail.com
>

Marcel, Johan, can either of you please comment on this patch? I've
sent a number of patches to linux-bluetooth in past 6 month all of
which were met with radio silence, so I am a bit worried that my
messages are being filtered out or dropped.

Thanks,
Andrey Smirnov


>  net/bluetooth/l2cap_core.c | 58 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index dfc1edb168b7..77b65870b064 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4216,6 +4216,49 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>         return err;
>  }
>
> +static inline int l2cap_config_rsp_unknown(struct l2cap_conn *conn,
> +                                          struct l2cap_chan *chan,
> +                                          const u8 *data,
> +                                          int len)
> +{
> +       char req[64];
> +
> +       if (!len || len > sizeof(req) -  sizeof(struct l2cap_conf_req))
> +               return -ECONNRESET;
> +
> +       while (len--) {
> +               const u8 option_type = *data++;
> +
> +               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 -ECONNRESET;
> +
> +               switch (option_type) {
> +               case L2CAP_CONF_RFC:
> +                       /* Clearing the following feature should
> +                        * prevent RFC option from being added next
> +                        * connection attempt
> +                        */
> +                       conn->feat_mask &= ~(L2CAP_FEAT_ERTM |
> +                                            L2CAP_FEAT_STREAMING);
> +                       break;
> +               default:
> +                       return -ECONNRESET;
> +               }
> +       }
> +
> +       len = l2cap_build_conf_req(chan, req, sizeof(req));
> +       l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_CONF_REQ, len, req);
> +       chan->num_conf_req++;
> +
> +       return 0;
> +}
> +
>  static inline int l2cap_config_rsp(struct l2cap_conn *conn,
>                                    struct l2cap_cmd_hdr *cmd, u16 cmd_len,
>                                    u8 *data)
> @@ -4271,6 +4314,21 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
>                 }
>                 goto done;
>
> +       case L2CAP_CONF_UNKNOWN:
> +               if (chan->num_conf_rsp <= L2CAP_CONF_MAX_CONF_RSP) {
> +                       if (l2cap_config_rsp_unknown(conn, chan, rsp->data,
> +                                                    len) < 0) {
> +                               l2cap_send_disconn_req(chan, ECONNRESET);
> +                               goto done;
> +                       }
> +                       break;
> +               }
> +               /* Once, chan->num_conf_rsp goes above
> +                * L2CAP_CONF_MAX_CONF_RSP we want to go down all the
> +                * way to default label (just like L2CAP_CONF_UNACCEPT
> +                * below)
> +                */
> +               /* fall through */
>         case L2CAP_CONF_UNACCEPT:
>                 if (chan->num_conf_rsp <= L2CAP_CONF_MAX_CONF_RSP) {
>                         char req[64];
> --
> 2.21.0
>