linux-kernel - Re: [PATCH v2] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 15 Jul 2014 15:06:00 +0200
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Jiada Wang <jiada_wang@...tor.com>
Cc:	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	Linux Bluetooth mailing list 
	<linux-bluetooth@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, Dean_Jenkins@...tor.com
Subject: Re: [PATCH v2] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer

Hi Jiada,

>>> 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 <djenkins@...sta.com>
>>> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>
>>> ---
>>> drivers/bluetooth/hci_bcsp.c | 94 ++++++++++++++++++++++++++++----------------
>>> 1 file changed, 60 insertions(+), 34 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
>>> index 21cc45b..0f4664d 100644
>>> --- a/drivers/bluetooth/hci_bcsp.c
>>> +++ b/drivers/bluetooth/hci_bcsp.c
>>> @@ -478,13 +478,29 @@ 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
>>> +			 */
>> 
>> Comment style is wrong.
>> 
>> 	/* aaa
>> 	 * bbb
>> 	 */
>> 
>>> +			BT_ERR ("Out-of-order packet arrived, got %u expected %u",
>>> +				bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>> 
>> It is BT_ERR(" and not BT_ERR (".
>> 
>>> +
>>> +			/* do not process out-of-order packet payload */
>>> +			pass_up = 2;
>>> +		}
>>> +
>>> +		/* send current txack value to all recieved reliable packets */
>>> +		bcsp->txack_req = 1;
>>> 
>>> 		/* If needed, transmit an ack pkt */
>>> 		hci_uart_tx_wakeup(hu);
>>> @@ -493,26 +509,36 @@ 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 recieved ACK indications,
>>> +	 * including those from out-of-order packets
>>> +	 */
>> 
>> Same here. Please fix comment style.
>> 
>>> 	bcsp_pkt_cull(bcsp);
>>> -	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) {
>>> +
>>> +	if (pass_up != 2) {
>>> +		if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>>> +				bcsp->rx_skb->data[0] & 0x80) {
>> 
>> Fix indentation here.
> 
> Can you tell me what should be the correct indentation.

		if ((bcsp->... &&
		    (bcsp->... )) {

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/