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
  0 siblings, 1 reply; 3+ 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] 3+ 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
  0 siblings, 1 reply; 3+ 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] 3+ 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
  0 siblings, 0 replies; 3+ 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] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ 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

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