Linux-Bluetooth Archive on lore.kernel.org
 help / Atom feed
* [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
@ 2019-02-08  2:58 Andrey Smirnov
  2019-02-18 12:58 ` Marcel Holtmann
  2019-04-23 20:08 ` Marcel Holtmann
  0 siblings, 2 replies; 6+ messages in thread
From: Andrey Smirnov @ 2019-02-08  2:58 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Andrey Smirnov, Pierre-Loup A . Griffais, Florian Dollinger,
	Marcel Holtmann, Johan Hedberg, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
  2019-02-08  2:58 [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN Andrey Smirnov
@ 2019-02-18 12:58 ` Marcel Holtmann
  2019-02-19  4:57   ` Andrey Smirnov
  2019-04-23 20:08 ` Marcel Holtmann
  1 sibling, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2019-02-18 12:58 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-bluetooth, Pierre-Loup A . Griffais, Florian Dollinger,
	Johan Hedberg, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
  2019-02-18 12:58 ` Marcel Holtmann
@ 2019-02-19  4:57   ` Andrey Smirnov
  2019-03-20 23:08     ` Andrey Smirnov
  0 siblings, 1 reply; 6+ messages in thread
From: Andrey Smirnov @ 2019-02-19  4:57 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Pierre-Loup A . Griffais, Florian Dollinger,
	Johan Hedberg, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
  2019-02-19  4:57   ` Andrey Smirnov
@ 2019-03-20 23:08     ` Andrey Smirnov
  2019-04-22 20:06       ` Florian Dollinger
  0 siblings, 1 reply; 6+ messages in thread
From: Andrey Smirnov @ 2019-03-20 23:08 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Pierre-Loup A . Griffais, Florian Dollinger,
	Johan Hedberg, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
  2019-03-20 23:08     ` Andrey Smirnov
@ 2019-04-22 20:06       ` Florian Dollinger
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Dollinger @ 2019-04-22 20:06 UTC (permalink / raw)
  To: Andrey Smirnov, Marcel Holtmann
  Cc: linux-bluetooth, Pierre-Loup A . Griffais, Johan Hedberg, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN
  2019-02-08  2:58 [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN Andrey Smirnov
  2019-02-18 12:58 ` Marcel Holtmann
@ 2019-04-23 20:08 ` Marcel Holtmann
  1 sibling, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2019-04-23 20:08 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: linux-bluetooth, Pierre-Loup A . Griffais, Florian Dollinger,
	Johan Hedberg, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08  2:58 [RFC] Bluetooth: Retry configure request if result is L2CAP_CONF_UNKNOWN Andrey Smirnov
2019-02-18 12:58 ` Marcel Holtmann
2019-02-19  4:57   ` Andrey Smirnov
2019-03-20 23:08     ` Andrey Smirnov
2019-04-22 20:06       ` Florian Dollinger
2019-04-23 20:08 ` Marcel Holtmann

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox