All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes
@ 2016-09-07 14:15 Dean Jenkins
  2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: linux-bluetooth

This patchset contains the following commits:

Dean Jenkins (1):
  Bluetooth: BCSP fails to ACK re-transmitted frames from the peer

Deepak Das (1):
  Bluetooth: Prevent scheduling of work after hci_uart_tty_close()

Vignesh Raman (2):
  Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking
  Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call

 drivers/bluetooth/hci_bcsp.c  | 82 +++++++++++++++++++++++++------------------
 drivers/bluetooth/hci_ldisc.c | 63 ++++++++++++++++++++++-----------
 drivers/bluetooth/hci_uart.h  |  3 ++
 3 files changed, 94 insertions(+), 54 deletions(-)

Background information
----------------------

These 4 commits were originally developed and validated on an ARM i.MX6 based
commercial project running a highly modified 3.14 Linux kernel. The commits were
needed to improve BCSP recovery and to avoid kernel crashes in the Bluetooth
sub-system start-up and shutdown scenarios.

The commits are used with a UART based Bluetooth Radio Module that uses BCSP.
This means the commits should have no impact on USB based Bluetooth Radio
Modules which are typically used on x86 based computer systems.

The commits have been forward-ported to bluetooth-next master branch HEAD
(4e9a68b fakelb: fix schedule while atomic)

The commits were also sanity tested on top of linux-stable v4.7.2 on a 64-bit
x86 Linux laptop with a USB to UART based Bluetooth Radio Module. The sanity
tests used l2ping to confirm that a remote Bluetooth smartphone could be
contacted.

Commit details
--------------

The following 2 commits are a pair and needs discussion in the community:

1. "Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking"

This patch fixes a kernel NULL pointer dereference crash when starting-up the
Bluetooth sub-system.

There is a flaw that HCI_UART_PROTO_SET is set before hci_uart_set_proto() is
successfully run to set the hu->proto->recv function pointer. This allows the
UART driver to attempt to process RX characters before the recv function pointer
has been set so causing a NULL pointer dereference crash.

The commit modifies the implementation to only set HCI_UART_PROTO_SET after
hci_uart_set_proto() has successfully executed.

It is thought that the following commit introduced in kernel v4.7-rc1
84cb3df0 "Bluetooth: hci_ldisc: Fix null pointer derefence in case of early data"
fixes the same crash by adding HCI_UART_PROTO_READY.

A doubt is which solution is most effective which might mean the proposed
patch is now redundant. However, both solutions can co-exist. This needs
discussion in the community.


2. "Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call"

This patch avoids a race condition by adding a mutex to hci_uart_tty_ioctl()
to prevent concurrency.

It is unclear whether this patch is now redundant due to kernel v4.7-rc1 commit
84cb3df0 "Bluetooth: hci_ldisc: Fix null pointer derefence in case of early data"

This needs discussion in the community.


3. "Bluetooth: BCSP fails to ACK re-transmitted frames from the peer"

This patch had previously been released to the mailing list in 2014 but somehow
never got into bluetooth-next at that time. Probably, there was an oversight in
keeping track of the patch to ensure it got reviewed by the maintainer.

The idea of this patch to make sure that the BCSP header of all received BCSP
frames from the Bluetooth Radio Module are consumed. The BCSP header contains
sequence counters for reliable transmitted, and received acknowledgment frames.
The acknowledgment counter is received in either reliable or unreliable frames.
These 2 independent counters are needed for a duplex link to track
acknowledgments and to trigger retransmissions.

In the local BCSP peer, each new reliable transmit frame increments the local TX
sequence number (modulo 8). The local BCSP peer has a window size of 4 so must
not transmit any new reliable frames until less than 4 acknowledgments are
pending. When a reliable BCSP frame is received, the remote BCSP peer's TX
sequence number is read and checked, and an acknowledgment frame should be sent
back to confirm the remote BCSP peer's transmission status.

The current implementation has a flaw because when the received reliable BCSP
frame does not have the expected remote TX sequence counter value, it causes the
whole frame to be dropped and causes the so-called "Out-of-order packet arrived"
error message to be generated. This incorrect behaviour means that the local
BCSP peer's acknowledgment counter number is not processed meaning that the
remote BCSP peer may have acknowledged the local BCSP peer's transmission but
the indication is ignored by the local BCSP peer. This can cause the local BCSP
peer to unnecessarily re-transmit. In addition the remote BCSP peer is not sent
an acknowledgment frame to update its transmission status. This can cause the
remote BCSP peer to unnecessarily retransmit frames.

In other words, the current implementation is weak in sending acknowledgments
in response to already received frames.

The flaw is observed with a poor performing UART driver which causes occasional
corruption of received or transmitted frames. BCSP is designed to compensate
for such issues by using its recovery mechanism and the flaw is in the
recovery mechanism.

The flaw can be triggered when the local BCSP peer fails to receive a reliable
frame from the remote BCSP peer so that reception of the next new reliable frame
from the remote BCSP peer causes the received transmission counter to skip over
(modulo 8) a value due to the missing frame. This generates the so-called
"Out-of-order packet arrived" error message but the acknowledgment counter
in the received frame's BCSP header is valid and should be processed and not
ignored. This protocol failure can be recovered when the local BCSP peer
retransmits an unacknowledged frame and the remote BCSP peer responds with an
acknowledgment using an unreliable frame so avoids the flawed code.

Alternatively, the flaw is triggered when the local BCSP peer tries to send
an acknowledgment frame to the remote BCSP peer but the remote BCSP peer failed
to receive the acknowledgment. This causes the local BCSP peer to increment
(modulo 8) the expected next remote BCSP peer's transmission sequence counter.
When the remote BCSP peer retransmits the unacknowledged frame, the local BCSP
peer is expecting the next counter value so causing the so-called
"Out-of-order packet arrived" error message to be generated. This causes the
received BCSP frame to be dropped so the local BCSP peer fails to send an
acknowledgment frame. This protocol failure due to the flaw may be
unrecoverable. However, recovery is sometimes possible when the remote BCSP peer
resends multiple unacknowledged frames so the next expected counter value is
seen so gets past the flawed code.


4. "Bluetooth: Prevent scheduling of work after hci_uart_tty_close()"

This patch prevents adding a transmission work item to the hu->write_work work
queue during execution of hci_uart_tty_close(). A kernel crash has been
observed whist BCSP was scheduling a retransmission and shutdown of the HCI UART
was in progress.

The fix introduces a new HCI_UART proto flag bit called HCI_UART_UNREGISTERING
and adds a spinlock to hci_uart_tx_wakeup() to force that function to run
consecutively with respect to hci_uart_tty_close().

The git commit text of the patch gives a fuller explanation so please read
that.

-- 
2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking
  2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
  2016-09-07 14:18   ` Marcel Holtmann
  2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: linux-bluetooth

From: Vignesh Raman <Vignesh_Raman@mentor.com>

Here is a NULL pointer dereference that causes a Bluetooth crash:

(hci_uart_tty_receive+0x0/0x84 [hci_uart]) from (flush_to_ldisc+0x104/0x190)
(flush_to_ldisc+0x0/0x190) from (tty_flip_buffer_push+0x50/0x5c)
(tty_flip_buffer_push+0x0/0x5c) from (imx_rxint+0x228/0x23c)
(imx_rxint+0x0/0x23c) from (imx_int+0xa8/0x174)
(imx_int+0x0/0x174) from (handle_irq_event_percpu+0x90/0x280)
(handle_irq_event_percpu+0x0/0x280) from (handle_irq_event+0x44/0x64)
(handle_irq_event+0x0/0x64) from (handle_fasteoi_irq+0xc0/0x108)
(handle_fasteoi_irq+0x0/0x108) from (generic_handle_irq+0x28/0x38)
(generic_handle_irq+0x0/0x38) from (handle_IRQ+0x6c/0x94)
(handle_IRQ+0x0/0x94) from (gic_handle_irq+0x44/0x68)
(gic_handle_irq+0x0/0x68) from (__irq_svc+0x40/0x70)
(bcsp_open+0x0/0xcc [hci_uart]) from (hci_uart_tty_ioctl+0xa8/0x238 [hci_uart])
(hci_uart_tty_ioctl+0x0/0x238 [hci_uart]) from (tty_ioctl+0xa88/0xae8)
(tty_ioctl+0x0/0xae8) from (vfs_ioctl+0x30/0x44)
(vfs_ioctl+0x0/0x44) from (do_vfs_ioctl+0x53c/0x590)
(do_vfs_ioctl+0x0/0x590) from (sys_ioctl+0x40/0x68)
(sys_ioctl+0x0/0x68) from (ret_fast_syscall+0x0/0x30)

It can be seen that bcsp_open() is pre-empted by an interrupt relating
to reception of UART characters. bcsp_open() is called from
hci_uart_set_proto() which is called from hci_uart_tty_ioctl() due to
HCIUARTSETPROTO handling.

The crash occurs in hci_uart_tty_receive() and analysis shows:
	if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
		return;

	spin_lock(&hu->rx_lock);
	hu->proto->recv(hu, (void *) data, count);

The crash occurs in the dereference hu->proto->recv. Presumably no recv
function has been set and a NULL pointer dereference occurs.

Note that the flag HCI_UART_PROTO_SET being clear should prevent the failure
but in fact fails. This indicates that HCI_UART_PROTO_SET is in the set state.

The HCI_UART_PROTO_SET locking fails because HCI_UART_PROTO_SET is put into
the set state in hci_uart_tty_ioctl() BEFORE calling hci_uart_set_proto()
to set the recv function pointer. Therefore, there is a race condition.

The solution is to set the flag HCI_UART_PROTO_SET after successfully
calling hci_uart_set_proto() in the HCIUARTSETPROTO handling in
hci_uart_tty_ioctl(). This will prevent hci_uart_tty_receive() from
performing the recv function pointer dereference until hci_uart_set_proto()
has set the recv function pointer.

Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Mark Craske <Mark_Craske@mentor.com>
---
 drivers/bluetooth/hci_ldisc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda9739..1e338d5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -695,12 +695,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
 
 	switch (cmd) {
 	case HCIUARTSETPROTO:
-		if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) {
+		if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
 			err = hci_uart_set_proto(hu, arg);
-			if (err) {
-				clear_bit(HCI_UART_PROTO_SET, &hu->flags);
+			if (!err)
+				set_bit(HCI_UART_PROTO_SET, &hu->flags);
+			else
 				return err;
-			}
 		} else
 			return -EBUSY;
 		break;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call
  2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
  2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
  2016-09-07 14:20   ` Marcel Holtmann
  2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
  2016-09-07 14:15 ` [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close() Dean Jenkins
  3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: linux-bluetooth

From: Vignesh Raman <Vignesh_Raman@mentor.com>

Add global mutex lock to prevent reentrancy in the hci_uart_tty_ioctl()
ioctl handling. This prevents concurrency of all function calls within
hci_uart_tty_ioctl handling including hci_uart_set_proto().

Also removed multiple return statements in hci_uart_tty_ioctl call and
added a single return statement since the mutex needs to be unlocked
before exiting the function.

Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
 drivers/bluetooth/hci_ldisc.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 1e338d5..cf1883d 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -51,6 +51,8 @@
 
 #define VERSION "2.3"
 
+static DEFINE_MUTEX(ioctl_mutex);
+
 static const struct hci_uart_proto *hup[HCI_UART_MAX_PROTO];
 
 int hci_uart_register_proto(const struct hci_uart_proto *p)
@@ -685,7 +687,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
 			      unsigned int cmd, unsigned long arg)
 {
 	struct hci_uart *hu = tty->disc_data;
-	int err = 0;
+	int result = 0;
 
 	BT_DBG("");
 
@@ -693,45 +695,52 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
 	if (!hu)
 		return -EBADF;
 
+	mutex_lock(&ioctl_mutex);
 	switch (cmd) {
 	case HCIUARTSETPROTO:
 		if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
-			err = hci_uart_set_proto(hu, arg);
-			if (!err)
+			result = hci_uart_set_proto(hu, arg);
+			if (!result)
 				set_bit(HCI_UART_PROTO_SET, &hu->flags);
-			else
-				return err;
 		} else
-			return -EBUSY;
+			result = -EBUSY;
 		break;
 
 	case HCIUARTGETPROTO:
 		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
-			return hu->proto->id;
-		return -EUNATCH;
+			result = hu->proto->id;
+		else
+			result = -EUNATCH;
+		break;
 
 	case HCIUARTGETDEVICE:
 		if (test_bit(HCI_UART_REGISTERED, &hu->flags))
-			return hu->hdev->id;
-		return -EUNATCH;
+			result = hu->hdev->id;
+		else
+			result = -EUNATCH;
+		break;
 
 	case HCIUARTSETFLAGS:
 		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
-			return -EBUSY;
-		err = hci_uart_set_flags(hu, arg);
-		if (err)
-			return err;
+			result = -EBUSY;
+		else {
+			result = hci_uart_set_flags(hu, arg);
+			if (result)
+				return result;
+		}
 		break;
 
 	case HCIUARTGETFLAGS:
-		return hu->hdev_flags;
+		result = hu->hdev_flags;
+		break;
 
 	default:
-		err = n_tty_ioctl_helper(tty, file, cmd, arg);
+		result = n_tty_ioctl_helper(tty, file, cmd, arg);
 		break;
 	}
+	mutex_unlock(&ioctl_mutex);
 
-	return err;
+	return result;
 }
 
 /*
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
  2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
  2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
  2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
  2016-09-07 14:22   ` Marcel Holtmann
  2016-09-07 14:15 ` [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close() Dean Jenkins
  3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: linux-bluetooth

Send an ACK frame with the current txack value in response to
every received reliable frame unless a TX reliable frame is being
sent. This modification allows re-transmitted frames from the remote
peer to be acknowledged rather than ignored. It means that the remote
peer knows which frame number to start re-transmitting from.

Without this modification, the recovery time to a missing frame
from the remote peer was unnecessarily being extended because the
headers of the out of order reliable frames were being discarded rather
than being processed. The frame headers of received frames will
indicate whether the local peer's transmissions have been
acknowledged by the remote peer. Therefore, the local peer may
unnecessarily re-transmit despite the remote peer already indicating
that the frame had been acknowledged in out of order reliable frame.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
 drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index d7d23ce..739ae59 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
 static void bcsp_complete_rx_pkt(struct hci_uart *hu)
 {
 	struct bcsp_struct *bcsp = hu->priv;
-	int pass_up;
+	int pass_up = 0;
 
 	if (bcsp->rx_skb->data[0] & 0x80) {	/* reliable pkt */
 		BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
-		bcsp->rxseq_txack++;
-		bcsp->rxseq_txack %= 0x8;
-		bcsp->txack_req    = 1;
+
+		/* check the rx sequence number is as expected */
+		if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
+			bcsp->rxseq_txack++;
+			bcsp->rxseq_txack %= 0x8;
+		} else {
+			/* handle re-transmitted packet or
+			   when packet was missed */
+			BT_ERR("Out-of-order packet arrived, got %u expected %u",
+				bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
+
+			/* do not process out-of-order packet payload */
+			pass_up = 2;
+		}
+
+		/* send current txack value to all received reliable packets */
+		bcsp->txack_req = 1;
 
 		/* If needed, transmit an ack pkt */
 		hci_uart_tx_wakeup(hu);
@@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
 	bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
 	BT_DBG("Request for pkt %u from card", bcsp->rxack);
 
+	/* handle received ACK indications,
+	   including those from out-of-order packets */
 	bcsp_pkt_cull(bcsp);
-	if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
-			bcsp->rx_skb->data[0] & 0x80) {
-		hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
-		pass_up = 1;
-	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
-			bcsp->rx_skb->data[0] & 0x80) {
-		hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
-		pass_up = 1;
-	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
-		hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
-		pass_up = 1;
-	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
-			!(bcsp->rx_skb->data[0] & 0x80)) {
-		bcsp_handle_le_pkt(hu);
-		pass_up = 0;
-	} else
-		pass_up = 0;
-
-	if (!pass_up) {
+
+	if (pass_up != 2) {
+		if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
+				(bcsp->rx_skb->data[0] & 0x80)) {
+			bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
+			pass_up = 1;
+		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
+				(bcsp->rx_skb->data[0] & 0x80)) {
+			bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
+			pass_up = 1;
+		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
+			bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
+			pass_up = 1;
+		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
+				!(bcsp->rx_skb->data[0] & 0x80)) {
+			bcsp_handle_le_pkt(hu);
+			pass_up = 0;
+		} else {
+			pass_up = 0;
+		}
+	}
+
+	if (pass_up == 0) {
 		struct hci_event_hdr hdr;
 		u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
 
@@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
 			}
 		} else
 			kfree_skb(bcsp->rx_skb);
-	} else {
+	} else if (pass_up == 1) {
 		/* Pull out BCSP hdr */
 		skb_pull(bcsp->rx_skb, 4);
 
 		hci_recv_frame(hu->hdev, bcsp->rx_skb);
+	} else {
+		/* ignore packet payload of already ACKed re-transmitted
+		   packets or when a packet was missed in the BCSP window */
+		kfree_skb(bcsp->rx_skb);
 	}
 
 	bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
@@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
 				bcsp->rx_count = 0;
 				continue;
 			}
-			if (bcsp->rx_skb->data[0] & 0x80	/* reliable pkt */
-						&& (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
-				BT_ERR("Out-of-order packet arrived, got %u expected %u",
-					bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
-
-				kfree_skb(bcsp->rx_skb);
-				bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
-				bcsp->rx_count = 0;
-				continue;
-			}
 			bcsp->rx_state = BCSP_W4_DATA;
 			bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
 					(bcsp->rx_skb->data[2] << 4);	/* May be 0 */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close()
  2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
                   ` (2 preceding siblings ...)
  2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
  3 siblings, 0 replies; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
  To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: linux-bluetooth

From: Deepak Das <Deepak_Das@mentor.com>

There is a work queue hu->write_work that can be scheduled to handle
a transmission work item in BCSP scenarios that are described below.

In these scenarios, the payload of a BCSP frame can contain a HCI
message as follows:

a) Normal sending of a HCI message within a BCSP frame from upper
layers via hci_uart_send_frame().

b) BCSP retransmission due to a failure to get a BCSP ACK indication.
This resends an old HCI message triggered by the BCSP timer timeout event
handled by bcsp_timed_event().

c) Sending of a BCSP ACK frame in response to a received reliable BCSP
frame is handled in bcsp_complete_rx_pkt() which allows an empty payload
(no HCI message) to be sent.

A good reproduction scenario of the crash is for a bad UART driver to
corrupt or cause a total loss of incoming BCSP frames. This leads to a link
failure so the BCSP timer is periodically triggered in an attempt to
retransmit unacknowledged frames. Then upper layers request the HCI UART
to close immediately before the BCSP timer event is handled. This causes
a race condition between closing the HCI UART and retransmitting a BCSP
frame.

The current solution is using cancel_work_sync(&hu->write_work); within
hci_uart_tty_close() in an attempt to cleanse the work queue of the
transmission work item.

It should be noted that a transmission work item is scheduled into the work
queue by hci_uart_tx_wakeup() which can be running in an atomic context
such as a UART driver interrupt thread or in a process context. Therefore,
it is possible for hci_uart_tx_wakeup() to interrupt or run concurrently
with hci_uart_tty_close().

In addition, hu->proto->close(hu) calls bcsp_close() which frees the
various queues of messages and deletes the BCSP timer.

The current solution has a flaw because hci_uart_tty_close() fails to
inhibit the asynchronous b) and c) transmission events from adding a work
item to the work queue AFTER cancel_work_sync(&hu->write_work); has been
executed. A crash occurs after bcsp_close() has freed the message queues
which are subsequently accessed when the work queue work item runs
hci_uart_write_work(). Therefore, there is a race condition.

This flaw was masked as typically retransmissions should be rare and
Bluetooth has a low sub-system rate of being actively shutdown.

This fix introduces a new HCI_UART proto flag bit called
HCI_UART_UNREGISTERING to indicate the unregistering state of the HCI UART.
This flag is used to prevent the addition of a transmission work item after
hci_uart_tty_close() starts to run.

Note that a spinlock is necessary because it is not possible to check
the state of the atomic flag and to schedule the work queue in an atomic
sequence of operations in hci_uart_tx_wakeup(). This means that the flag
must be prevented from changing state after the flag is checked and before
the work queue is scheduled in hci_uart_tx_wakeup().

Signed-off-by: Deepak Das <Deepak_Das@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
Signed-off-by: Dean_Jenkins@mentor.com
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
 drivers/bluetooth/hci_ldisc.c | 16 +++++++++++++++-
 drivers/bluetooth/hci_uart.h  |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index cf1883d..ddf4cda 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -125,15 +125,22 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
 
 int hci_uart_tx_wakeup(struct hci_uart *hu)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&hu->schedule_lock, flags);
+	if (test_bit(HCI_UART_UNREGISTERING, &hu->flags))
+		goto no_schedule;
 	if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
 		set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
-		return 0;
+		goto no_schedule;
 	}
 
 	BT_DBG("");
 
 	schedule_work(&hu->write_work);
 
+no_schedule:
+	spin_unlock_irqrestore(&hu->schedule_lock, flags);
 	return 0;
 }
 
@@ -464,6 +471,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	INIT_WORK(&hu->init_ready, hci_uart_init_work);
 	INIT_WORK(&hu->write_work, hci_uart_write_work);
 
+	spin_lock_init(&hu->schedule_lock);
+
 	/* Flush any pending characters in the driver */
 	tty_driver_flush_buffer(tty);
 
@@ -479,6 +488,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 {
 	struct hci_uart *hu = tty->disc_data;
 	struct hci_dev *hdev;
+	unsigned long flags;
 
 	BT_DBG("tty %p", tty);
 
@@ -488,6 +498,10 @@ static void hci_uart_tty_close(struct tty_struct *tty)
 	if (!hu)
 		return;
 
+	spin_lock_irqsave(&hu->schedule_lock, flags);
+	set_bit(HCI_UART_UNREGISTERING, &hu->flags);
+	spin_unlock_irqrestore(&hu->schedule_lock, flags);
+
 	hdev = hu->hdev;
 	if (hdev)
 		hci_uart_close(hdev);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1..27a7708 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -88,6 +88,8 @@ struct hci_uart {
 	struct sk_buff		*tx_skb;
 	unsigned long		tx_state;
 
+	/* prevent scheduling work during close */
+	spinlock_t		schedule_lock;
 	unsigned int init_speed;
 	unsigned int oper_speed;
 };
@@ -96,6 +98,7 @@ struct hci_uart {
 #define HCI_UART_PROTO_SET	0
 #define HCI_UART_REGISTERED	1
 #define HCI_UART_PROTO_READY	2
+#define HCI_UART_UNREGISTERING	3
 
 /* TX states  */
 #define HCI_UART_SENDING	1
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking
  2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
@ 2016-09-07 14:18   ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:18 UTC (permalink / raw)
  To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth

Hi Dean,

> Here is a NULL pointer dereference that causes a Bluetooth crash:
> 
> (hci_uart_tty_receive+0x0/0x84 [hci_uart]) from (flush_to_ldisc+0x104/0x190)
> (flush_to_ldisc+0x0/0x190) from (tty_flip_buffer_push+0x50/0x5c)
> (tty_flip_buffer_push+0x0/0x5c) from (imx_rxint+0x228/0x23c)
> (imx_rxint+0x0/0x23c) from (imx_int+0xa8/0x174)
> (imx_int+0x0/0x174) from (handle_irq_event_percpu+0x90/0x280)
> (handle_irq_event_percpu+0x0/0x280) from (handle_irq_event+0x44/0x64)
> (handle_irq_event+0x0/0x64) from (handle_fasteoi_irq+0xc0/0x108)
> (handle_fasteoi_irq+0x0/0x108) from (generic_handle_irq+0x28/0x38)
> (generic_handle_irq+0x0/0x38) from (handle_IRQ+0x6c/0x94)
> (handle_IRQ+0x0/0x94) from (gic_handle_irq+0x44/0x68)
> (gic_handle_irq+0x0/0x68) from (__irq_svc+0x40/0x70)
> (bcsp_open+0x0/0xcc [hci_uart]) from (hci_uart_tty_ioctl+0xa8/0x238 [hci_uart])
> (hci_uart_tty_ioctl+0x0/0x238 [hci_uart]) from (tty_ioctl+0xa88/0xae8)
> (tty_ioctl+0x0/0xae8) from (vfs_ioctl+0x30/0x44)
> (vfs_ioctl+0x0/0x44) from (do_vfs_ioctl+0x53c/0x590)
> (do_vfs_ioctl+0x0/0x590) from (sys_ioctl+0x40/0x68)
> (sys_ioctl+0x0/0x68) from (ret_fast_syscall+0x0/0x30)
> 
> It can be seen that bcsp_open() is pre-empted by an interrupt relating
> to reception of UART characters. bcsp_open() is called from
> hci_uart_set_proto() which is called from hci_uart_tty_ioctl() due to
> HCIUARTSETPROTO handling.
> 
> The crash occurs in hci_uart_tty_receive() and analysis shows:
> 	if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
> 		return;
> 
> 	spin_lock(&hu->rx_lock);
> 	hu->proto->recv(hu, (void *) data, count);
> 
> The crash occurs in the dereference hu->proto->recv. Presumably no recv
> function has been set and a NULL pointer dereference occurs.
> 
> Note that the flag HCI_UART_PROTO_SET being clear should prevent the failure
> but in fact fails. This indicates that HCI_UART_PROTO_SET is in the set state.
> 
> The HCI_UART_PROTO_SET locking fails because HCI_UART_PROTO_SET is put into
> the set state in hci_uart_tty_ioctl() BEFORE calling hci_uart_set_proto()
> to set the recv function pointer. Therefore, there is a race condition.
> 
> The solution is to set the flag HCI_UART_PROTO_SET after successfully
> calling hci_uart_set_proto() in the HCIUARTSETPROTO handling in
> hci_uart_tty_ioctl(). This will prevent hci_uart_tty_receive() from
> performing the recv function pointer dereference until hci_uart_set_proto()
> has set the recv function pointer.
> 
> Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Mark Craske <Mark_Craske@mentor.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index dda9739..1e338d5 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -695,12 +695,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
> 
> 	switch (cmd) {
> 	case HCIUARTSETPROTO:
> -		if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> +		if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> 			err = hci_uart_set_proto(hu, arg);
> -			if (err) {
> -				clear_bit(HCI_UART_PROTO_SET, &hu->flags);
> +			if (!err)
> +				set_bit(HCI_UART_PROTO_SET, &hu->flags);
> +			else
> 				return err;
> -			}

can we flip this around.

		if (err)
			return err;

		/* Comment why this is correct thing to do */
		set_bit(HCI_UART_PROTO_SET, &hu->flags);

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call
  2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
@ 2016-09-07 14:20   ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:20 UTC (permalink / raw)
  To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth

Hi Dean,

> Add global mutex lock to prevent reentrancy in the hci_uart_tty_ioctl()
> ioctl handling. This prevents concurrency of all function calls within
> hci_uart_tty_ioctl handling including hci_uart_set_proto().
> 
> Also removed multiple return statements in hci_uart_tty_ioctl call and
> added a single return statement since the mutex needs to be unlocked
> before exiting the function.

can we split this into two please. First change the error handling path and then introduce the mutex.

> 
> Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 1e338d5..cf1883d 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -51,6 +51,8 @@
> 
> #define VERSION "2.3"
> 
> +static DEFINE_MUTEX(ioctl_mutex);
> +
> static const struct hci_uart_proto *hup[HCI_UART_MAX_PROTO];
> 
> int hci_uart_register_proto(const struct hci_uart_proto *p)
> @@ -685,7 +687,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
> 			      unsigned int cmd, unsigned long arg)
> {
> 	struct hci_uart *hu = tty->disc_data;
> -	int err = 0;
> +	int result = 0;

I am fine keeping this err. I think renaming it to result does not help.

> 
> 	BT_DBG("");
> 
> @@ -693,45 +695,52 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
> 	if (!hu)
> 		return -EBADF;
> 
> +	mutex_lock(&ioctl_mutex);
> 	switch (cmd) {
> 	case HCIUARTSETPROTO:
> 		if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> -			err = hci_uart_set_proto(hu, arg);
> -			if (!err)
> +			result = hci_uart_set_proto(hu, arg);
> +			if (!result)
> 				set_bit(HCI_UART_PROTO_SET, &hu->flags);
> -			else
> -				return err;
> 		} else
> -			return -EBUSY;
> +			result = -EBUSY;
> 		break;
> 
> 	case HCIUARTGETPROTO:
> 		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
> -			return hu->proto->id;
> -		return -EUNATCH;
> +			result = hu->proto->id;
> +		else
> +			result = -EUNATCH;
> +		break;
> 
> 	case HCIUARTGETDEVICE:
> 		if (test_bit(HCI_UART_REGISTERED, &hu->flags))
> -			return hu->hdev->id;
> -		return -EUNATCH;
> +			result = hu->hdev->id;
> +		else
> +			result = -EUNATCH;
> +		break;
> 
> 	case HCIUARTSETFLAGS:
> 		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
> -			return -EBUSY;
> -		err = hci_uart_set_flags(hu, arg);
> -		if (err)
> -			return err;
> +			result = -EBUSY;
> +		else {
> +			result = hci_uart_set_flags(hu, arg);
> +			if (result)
> +				return result;
> +		}
> 		break;
> 
> 	case HCIUARTGETFLAGS:
> -		return hu->hdev_flags;
> +		result = hu->hdev_flags;
> +		break;
> 
> 	default:
> -		err = n_tty_ioctl_helper(tty, file, cmd, arg);
> +		result = n_tty_ioctl_helper(tty, file, cmd, arg);
> 		break;
> 	}
> +	mutex_unlock(&ioctl_mutex);
> 
> -	return err;
> +	return result;
> }

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
  2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
@ 2016-09-07 14:22   ` Marcel Holtmann
  2016-09-08  9:41     ` Dean Jenkins
  0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:22 UTC (permalink / raw)
  To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth

Hi Dean,

> Send an ACK frame with the current txack value in response to
> every received reliable frame unless a TX reliable frame is being
> sent. This modification allows re-transmitted frames from the remote
> peer to be acknowledged rather than ignored. It means that the remote
> peer knows which frame number to start re-transmitting from.
> 
> Without this modification, the recovery time to a missing frame
> from the remote peer was unnecessarily being extended because the
> headers of the out of order reliable frames were being discarded rather
> than being processed. The frame headers of received frames will
> indicate whether the local peer's transmissions have been
> acknowledged by the remote peer. Therefore, the local peer may
> unnecessarily re-transmit despite the remote peer already indicating
> that the frame had been acknowledged in out of order reliable frame.
> 
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
> ---
> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index d7d23ce..739ae59 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> {
> 	struct bcsp_struct *bcsp = hu->priv;
> -	int pass_up;
> +	int pass_up = 0;
> 
> 	if (bcsp->rx_skb->data[0] & 0x80) {	/* reliable pkt */
> 		BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
> -		bcsp->rxseq_txack++;
> -		bcsp->rxseq_txack %= 0x8;
> -		bcsp->txack_req    = 1;
> +
> +		/* check the rx sequence number is as expected */
> +		if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
> +			bcsp->rxseq_txack++;
> +			bcsp->rxseq_txack %= 0x8;
> +		} else {
> +			/* handle re-transmitted packet or
> +			   when packet was missed */
> +			BT_ERR("Out-of-order packet arrived, got %u expected %u",
> +				bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
> +
> +			/* do not process out-of-order packet payload */
> +			pass_up = 2;
> +		}
> +
> +		/* send current txack value to all received reliable packets */
> +		bcsp->txack_req = 1;
> 
> 		/* If needed, transmit an ack pkt */
> 		hci_uart_tx_wakeup(hu);
> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> 	bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
> 	BT_DBG("Request for pkt %u from card", bcsp->rxack);
> 
> +	/* handle received ACK indications,
> +	   including those from out-of-order packets */
> 	bcsp_pkt_cull(bcsp);
> -	if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> -			bcsp->rx_skb->data[0] & 0x80) {
> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
> -		pass_up = 1;
> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
> -			bcsp->rx_skb->data[0] & 0x80) {
> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
> -		pass_up = 1;
> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
> -		pass_up = 1;
> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
> -			!(bcsp->rx_skb->data[0] & 0x80)) {
> -		bcsp_handle_le_pkt(hu);
> -		pass_up = 0;
> -	} else
> -		pass_up = 0;
> -
> -	if (!pass_up) {
> +
> +	if (pass_up != 2) {
> +		if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> +				(bcsp->rx_skb->data[0] & 0x80)) {

lets start using proper network subsystem coding style now.

Regards

Marcel

> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
> +			pass_up = 1;
> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
> +				(bcsp->rx_skb->data[0] & 0x80)) {
> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
> +			pass_up = 1;
> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
> +			pass_up = 1;
> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
> +				!(bcsp->rx_skb->data[0] & 0x80)) {
> +			bcsp_handle_le_pkt(hu);
> +			pass_up = 0;
> +		} else {
> +			pass_up = 0;
> +		}
> +	}
> +
> +	if (pass_up == 0) {
> 		struct hci_event_hdr hdr;
> 		u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
> 
> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> 			}
> 		} else
> 			kfree_skb(bcsp->rx_skb);
> -	} else {
> +	} else if (pass_up == 1) {
> 		/* Pull out BCSP hdr */
> 		skb_pull(bcsp->rx_skb, 4);
> 
> 		hci_recv_frame(hu->hdev, bcsp->rx_skb);
> +	} else {
> +		/* ignore packet payload of already ACKed re-transmitted
> +		   packets or when a packet was missed in the BCSP window */

Also use network subsystem comment style.

> +		kfree_skb(bcsp->rx_skb);
> 	}
> 
> 	bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> @@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
> 				bcsp->rx_count = 0;
> 				continue;
> 			}
> -			if (bcsp->rx_skb->data[0] & 0x80	/* reliable pkt */
> -						&& (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
> -				BT_ERR("Out-of-order packet arrived, got %u expected %u",
> -					bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
> -
> -				kfree_skb(bcsp->rx_skb);
> -				bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> -				bcsp->rx_count = 0;
> -				continue;
> -			}
> 			bcsp->rx_state = BCSP_W4_DATA;
> 			bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
> 					(bcsp->rx_skb->data[2] << 4);	/* May be 0 */

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
  2016-09-07 14:22   ` Marcel Holtmann
@ 2016-09-08  9:41     ` Dean Jenkins
  2016-09-08 10:28       ` Marcel Holtmann
  0 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-08  9:41 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth

Hi Marcel,

On 07/09/16 15:22, Marcel Holtmann wrote:
> Hi Dean,
>
>> Send an ACK frame with the current txack value in response to
>> every received reliable frame unless a TX reliable frame is being
>> sent. This modification allows re-transmitted frames from the remote
>> peer to be acknowledged rather than ignored. It means that the remote
>> peer knows which frame number to start re-transmitting from.
>>
>> Without this modification, the recovery time to a missing frame
>> from the remote peer was unnecessarily being extended because the
>> headers of the out of order reliable frames were being discarded rather
>> than being processed. The frame headers of received frames will
>> indicate whether the local peer's transmissions have been
>> acknowledged by the remote peer. Therefore, the local peer may
>> unnecessarily re-transmit despite the remote peer already indicating
>> that the frame had been acknowledged in out of order reliable frame.
>>
>> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
>> ---
>> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
>> 1 file changed, 48 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
>> index d7d23ce..739ae59 100644
>> --- a/drivers/bluetooth/hci_bcsp.c
>> +++ b/drivers/bluetooth/hci_bcsp.c
>> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
>> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> {
>> 	struct bcsp_struct *bcsp = hu->priv;
>> -	int pass_up;
>> +	int pass_up = 0;
>>
>> 	if (bcsp->rx_skb->data[0] & 0x80) {	/* reliable pkt */
>> 		BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
>> -		bcsp->rxseq_txack++;
>> -		bcsp->rxseq_txack %= 0x8;
>> -		bcsp->txack_req    = 1;
>> +
>> +		/* check the rx sequence number is as expected */
>> +		if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
>> +			bcsp->rxseq_txack++;
>> +			bcsp->rxseq_txack %= 0x8;
>> +		} else {
>> +			/* handle re-transmitted packet or
>> +			   when packet was missed */
>> +			BT_ERR("Out-of-order packet arrived, got %u expected %u",
>> +				bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>> +
>> +			/* do not process out-of-order packet payload */
>> +			pass_up = 2;
>> +		}
>> +
>> +		/* send current txack value to all received reliable packets */
>> +		bcsp->txack_req = 1;
>>
>> 		/* If needed, transmit an ack pkt */
>> 		hci_uart_tx_wakeup(hu);
>> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> 	bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
>> 	BT_DBG("Request for pkt %u from card", bcsp->rxack);
>>
>> +	/* handle received ACK indications,
>> +	   including those from out-of-order packets */
>> 	bcsp_pkt_cull(bcsp);
>> -	if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>> -			bcsp->rx_skb->data[0] & 0x80) {
>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
>> -		pass_up = 1;
>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>> -			bcsp->rx_skb->data[0] & 0x80) {
>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
>> -		pass_up = 1;
>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
>> -		pass_up = 1;
>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>> -			!(bcsp->rx_skb->data[0] & 0x80)) {
>> -		bcsp_handle_le_pkt(hu);
>> -		pass_up = 0;
>> -	} else
>> -		pass_up = 0;
>> -
>> -	if (!pass_up) {
>> +
>> +	if (pass_up != 2) {
>> +		if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>> +				(bcsp->rx_skb->data[0] & 0x80)) {
> lets start using proper network subsystem coding style now.

I think you are referring to the indentation style, right ?

I am aware of the checkpatch.pl --strict warning of
CHECK: Alignment should match open parenthesis

Would this style be acceptable so the 2nd line of arguments is one character
position to the right of the opening parenthesis of the 1st line by adding
spaces after tabs to do the alignment ?

+        if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
+            (bcsp->rx_skb->data[0] & 0x80)) {

>
> Regards
>
> Marcel
>
>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
>> +			pass_up = 1;
>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>> +				(bcsp->rx_skb->data[0] & 0x80)) {
>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
>> +			pass_up = 1;
>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
>> +			pass_up = 1;
>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>> +				!(bcsp->rx_skb->data[0] & 0x80)) {
>> +			bcsp_handle_le_pkt(hu);
>> +			pass_up = 0;
>> +		} else {
>> +			pass_up = 0;
>> +		}
>> +	}
>> +
>> +	if (pass_up == 0) {
>> 		struct hci_event_hdr hdr;
>> 		u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
>>
>> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> 			}
>> 		} else
>> 			kfree_skb(bcsp->rx_skb);
>> -	} else {
>> +	} else if (pass_up == 1) {
>> 		/* Pull out BCSP hdr */
>> 		skb_pull(bcsp->rx_skb, 4);
>>
>> 		hci_recv_frame(hu->hdev, bcsp->rx_skb);
>> +	} else {
>> +		/* ignore packet payload of already ACKed re-transmitted
>> +		   packets or when a packet was missed in the BCSP window */
> Also use network subsystem comment style.

Please can confirm which comment style is your preference because I have 
doubts.

Below is a list of possible comment styles (I think), please tell me 
which ones
are acceptable:

a)
/*
  * comment line #1
  * comment line #2
*/

b) I think you prefer this style aka "network subsystem style"
/* comment line #1
  * comment line #2
  */

c)
/* comment line #1 */
/* comment line #2 */

d) this style was used in our patch to match the style in the file
/* comment line #1
    comment line #2 */

drivers/bluetooth/hci_bcsp.c already contains styles b) and d)

If you agree, I can create some style clean-up commits for
drivers/bluetooth/hci_bcsp.c before applying our Bluetooth patch.

>
>> +		kfree_skb(bcsp->rx_skb);
>> 	}
>>
>> 	bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
>> @@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
>> 				bcsp->rx_count = 0;
>> 				continue;
>> 			}
>> -			if (bcsp->rx_skb->data[0] & 0x80	/* reliable pkt */
>> -						&& (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
>> -				BT_ERR("Out-of-order packet arrived, got %u expected %u",
>> -					bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>> -
>> -				kfree_skb(bcsp->rx_skb);
>> -				bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
>> -				bcsp->rx_count = 0;
>> -				continue;
>> -			}
>> 			bcsp->rx_state = BCSP_W4_DATA;
>> 			bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
>> 					(bcsp->rx_skb->data[2] << 4);	/* May be 0 */

Regards,
Dean

-- 
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
  2016-09-08  9:41     ` Dean Jenkins
@ 2016-09-08 10:28       ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-08 10:28 UTC (permalink / raw)
  To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth

Hi Dean,

>>> Send an ACK frame with the current txack value in response to
>>> every received reliable frame unless a TX reliable frame is being
>>> sent. This modification allows re-transmitted frames from the remote
>>> peer to be acknowledged rather than ignored. It means that the remote
>>> peer knows which frame number to start re-transmitting from.
>>> 
>>> Without this modification, the recovery time to a missing frame
>>> from the remote peer was unnecessarily being extended because the
>>> headers of the out of order reliable frames were being discarded rather
>>> than being processed. The frame headers of received frames will
>>> indicate whether the local peer's transmissions have been
>>> acknowledged by the remote peer. Therefore, the local peer may
>>> unnecessarily re-transmit despite the remote peer already indicating
>>> that the frame had been acknowledged in out of order reliable frame.
>>> 
>>> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
>>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>>> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
>>> ---
>>> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
>>> 1 file changed, 48 insertions(+), 34 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
>>> index d7d23ce..739ae59 100644
>>> --- a/drivers/bluetooth/hci_bcsp.c
>>> +++ b/drivers/bluetooth/hci_bcsp.c
>>> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
>>> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> {
>>> 	struct bcsp_struct *bcsp = hu->priv;
>>> -	int pass_up;
>>> +	int pass_up = 0;
>>> 
>>> 	if (bcsp->rx_skb->data[0] & 0x80) {	/* reliable pkt */
>>> 		BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
>>> -		bcsp->rxseq_txack++;
>>> -		bcsp->rxseq_txack %= 0x8;
>>> -		bcsp->txack_req    = 1;
>>> +
>>> +		/* check the rx sequence number is as expected */
>>> +		if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
>>> +			bcsp->rxseq_txack++;
>>> +			bcsp->rxseq_txack %= 0x8;
>>> +		} else {
>>> +			/* handle re-transmitted packet or
>>> +			   when packet was missed */
>>> +			BT_ERR("Out-of-order packet arrived, got %u expected %u",
>>> +				bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>>> +
>>> +			/* do not process out-of-order packet payload */
>>> +			pass_up = 2;
>>> +		}
>>> +
>>> +		/* send current txack value to all received reliable packets */
>>> +		bcsp->txack_req = 1;
>>> 
>>> 		/* If needed, transmit an ack pkt */
>>> 		hci_uart_tx_wakeup(hu);
>>> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> 	bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
>>> 	BT_DBG("Request for pkt %u from card", bcsp->rxack);
>>> 
>>> +	/* handle received ACK indications,
>>> +	   including those from out-of-order packets */
>>> 	bcsp_pkt_cull(bcsp);
>>> -	if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>>> -			bcsp->rx_skb->data[0] & 0x80) {
>>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
>>> -		pass_up = 1;
>>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>>> -			bcsp->rx_skb->data[0] & 0x80) {
>>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
>>> -		pass_up = 1;
>>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>>> -		hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
>>> -		pass_up = 1;
>>> -	} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>>> -			!(bcsp->rx_skb->data[0] & 0x80)) {
>>> -		bcsp_handle_le_pkt(hu);
>>> -		pass_up = 0;
>>> -	} else
>>> -		pass_up = 0;
>>> -
>>> -	if (!pass_up) {
>>> +
>>> +	if (pass_up != 2) {
>>> +		if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>>> +				(bcsp->rx_skb->data[0] & 0x80)) {
>> lets start using proper network subsystem coding style now.
> 
> I think you are referring to the indentation style, right ?
> 
> I am aware of the checkpatch.pl --strict warning of
> CHECK: Alignment should match open parenthesis
> 
> Would this style be acceptable so the 2nd line of arguments is one character
> position to the right of the opening parenthesis of the 1st line by adding
> spaces after tabs to do the alignment ?
> 
> +        if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> +            (bcsp->rx_skb->data[0] & 0x80)) {

yes, this is how it should look like.

>> 
>>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
>>> +			pass_up = 1;
>>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>>> +				(bcsp->rx_skb->data[0] & 0x80)) {
>>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
>>> +			pass_up = 1;
>>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>>> +			bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
>>> +			pass_up = 1;
>>> +		} else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>>> +				!(bcsp->rx_skb->data[0] & 0x80)) {
>>> +			bcsp_handle_le_pkt(hu);
>>> +			pass_up = 0;
>>> +		} else {
>>> +			pass_up = 0;
>>> +		}
>>> +	}
>>> +
>>> +	if (pass_up == 0) {
>>> 		struct hci_event_hdr hdr;
>>> 		u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
>>> 
>>> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> 			}
>>> 		} else
>>> 			kfree_skb(bcsp->rx_skb);
>>> -	} else {
>>> +	} else if (pass_up == 1) {
>>> 		/* Pull out BCSP hdr */
>>> 		skb_pull(bcsp->rx_skb, 4);
>>> 
>>> 		hci_recv_frame(hu->hdev, bcsp->rx_skb);
>>> +	} else {
>>> +		/* ignore packet payload of already ACKed re-transmitted
>>> +		   packets or when a packet was missed in the BCSP window */
>> Also use network subsystem comment style.
> 
> Please can confirm which comment style is your preference because I have doubts.
> 
> Below is a list of possible comment styles (I think), please tell me which ones
> are acceptable:
> 
> a)
> /*
> * comment line #1
> * comment line #2
> */
> 
> b) I think you prefer this style aka "network subsystem style"
> /* comment line #1
> * comment line #2
> */

Yes. That is what we are suppose to be using.

> 
> c)
> /* comment line #1 */
> /* comment line #2 */
> 
> d) this style was used in our patch to match the style in the file
> /* comment line #1
>   comment line #2 */
> 
> drivers/bluetooth/hci_bcsp.c already contains styles b) and d)
> 
> If you agree, I can create some style clean-up commits for
> drivers/bluetooth/hci_bcsp.c before applying our Bluetooth patch.

Go ahead if you have time for it.

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-09-08 10:28 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
2016-09-07 14:18   ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
2016-09-07 14:20   ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
2016-09-07 14:22   ` Marcel Holtmann
2016-09-08  9:41     ` Dean Jenkins
2016-09-08 10:28       ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close() Dean Jenkins

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.