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

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

[Andrey Smirnov]

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

 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) {
@@ -7067,6 +7108,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 	hcon->l2cap_data = conn;
 	conn->hcon = hci_conn_get(hcon);
 	conn->hchan = hchan;
+	conn->known_options = U32_MAX;
 
 	BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
 
-- 
2.20.1

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

[Marcel Holtmann]

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.

can you take a btmon -w trace.log protocol trace so that I can see where it fails. This seems a really odd behavior of the Xbox controller. We have to be careful in not breaking Bluetooth qualification to just workaround some buggy remote device.

Regards

Marcel

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

[Andrey Smirnov]

On Mon, Feb 18, 2019 at 4:58 AM 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.
>
> can you take a btmon -w trace.log protocol trace so that I can see where it fails. This seems a really odd behavior of the Xbox controller. We have to be careful in not breaking Bluetooth qualification to just workaround some buggy remote device.
>

Sure, n/p, both "failure" (behavior before this patch) and "success"
(behavior with the patch) cases on my machine are available here:

https://gist.github.com/ndreys/2b74094933601978e200af1ff0a55372

Let me know if that's not accessible to you.

Thanks,
Andrey Smirnov

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

[Andrey Smirnov]

On Mon, Feb 18, 2019 at 8:57 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Mon, Feb 18, 2019 at 4:58 AM 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.
> >
> > can you take a btmon -w trace.log protocol trace so that I can see where it fails. This seems a really odd behavior of the Xbox controller. We have to be careful in not breaking Bluetooth qualification to just workaround some buggy remote device.
> >
>
> Sure, n/p, both "failure" (behavior before this patch) and "success"
> (behavior with the patch) cases on my machine are available here:
>
> https://gist.github.com/ndreys/2b74094933601978e200af1ff0a55372
>
> Let me know if that's not accessible to you.
>

Marcel, did you have a chance to look at the logs?

Thanks,
Andrey Smirnov

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

[Florian Dollinger]

I think in essence this is the same as my patch from Jan 2018 here:
https://raw.githubusercontent.com/atar-axis/xpadneo/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch

Right? That's maybe why I am in CC :D
If yes, then I can fully confirm that this works as one would expect.

Let me copy&paste my patch description here:

---

The current L2CAP implementation does not change any options if the
other side respons with "unknown options", but does if "unaccepted
options" is the answer. It is up to the implementation to decide on the
effort spent on config negotiations, therefore the current
implementation is  correct at this point - but [...] devices like [the]
Xbox One S controller [is] not useable this way.
  A workaround for many users therefore is to disable_ertm, since this
is [in this case] the option which is unknown. I would prefer to try it
again with altered options instead of globally disable ERTM.

In result, I suggest the following patch. It simply adds a new case
(L2CAP_CONF_UNKNOWN), which does nothing but falling through to
L2CAP_CONF_UNACCEPT.

---

Cheers,
Florian Dollinger (atar-axis)

On 21.03.19 00:08, Andrey Smirnov wrote:
> On Mon, Feb 18, 2019 at 8:57 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>>
>> On Mon, Feb 18, 2019 at 4:58 AM 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.
>>>
>>> can you take a btmon -w trace.log protocol trace so that I can see where it fails. This seems a really odd behavior of the Xbox controller. We have to be careful in not breaking Bluetooth qualification to just workaround some buggy remote device.
>>>
>>
>> Sure, n/p, both "failure" (behavior before this patch) and "success"
>> (behavior with the patch) cases on my machine are available here:
>>
>> https://gist.github.com/ndreys/2b74094933601978e200af1ff0a55372
>>
>> Let me know if that's not accessible to you.
>>
>
> Marcel, did you have a chance to look at the logs?
>
> Thanks,
> Andrey Smirnov
>

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

[Marcel Holtmann]

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.

> 
> 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.

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.

Regards

Marcel

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

[Andrey Smirnov]

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