Hi Luiz,
On Sun, May 14, 2017 at 1:00 PM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Ricardo,
>
> On Sat, May 13, 2017 at 4:13 AM, Ricardo Salveti
> <ricardo.salveti@xxxxxxxxxx> wrote:
>> hci_sched_le only checks for the available le pkts before iterating over
>> the channel data queue, allowing hci data buffer overflow when quota is
>> larger than cnt (hci_chan_sent uses both le_cnt and acl_cnt when
>> calculating quota, both of which are only updated after hci_sched_le is
>> done with the channel data queue).
>>
>> Bug found when using wl1835mod (96boards HiKey) with multiple BT LE
>> connections:
>>> HCI Event: Number of Completed Packets (0x13) plen 5
>> Num handles: 1
>> Handle: 1025
>> Count: 2
>>> HCI Event: Data Buffer Overflow (0x1a) plen 1
>> Link type: ACL (0x01)
>>> HCI Event: Data Buffer Overflow (0x1a) plen 1
>> Link type: ACL (0x01)
>>> HCI Event: Data Buffer Overflow (0x1a) plen 1
>> Link type: ACL (0x01)
>>> HCI Event: Data Buffer Overflow (0x1a) plen 1
>> Link type: ACL (0x01)
>>> HCI Event: Data Buffer Overflow (0x1a) plen 1
>> Link type: ACL (0x01)
>>
>> Signed-off-by: Ricardo Salveti <ricardo.salveti@xxxxxxxxxx>
>> ---
>> net/bluetooth/hci_core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 0568677..58e9ab2 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -3938,7 +3938,7 @@ static void hci_sched_le(struct hci_dev *hdev)
>> tmp = cnt;
>> while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
>> u32 priority = (skb_peek(&chan->data_q))->priority;
>> - while (quote-- && (skb = skb_peek(&chan->data_q))) {
>> + while (cnt && quote-- && (skb = skb_peek(&chan->data_q))) {
>> BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
>> skb->len, skb->priority);
>>
>> --
>> 2.7.4
>
> This does indeed seems wrong but I think this can also affect the
> quote since hci_chan_sent does attempt to access the acl_cnt/le_cnt
> which may lead to return the wrong quote, though checking cnt would
> fix that as well, because of this perhaps we should actually have the
> cnt as a pointer so we can update the real cnt in place similarly to
> what is done for ACL_LINK/acl_cnt:
This is indeed better as it fixes quote and is similar to acl_cnt.
> net/bluetooth/hci_core.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7655b40..8c4c785 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3955,7 +3955,7 @@ static void hci_sched_le(struct hci_dev *hdev)
> {
> struct hci_chan *chan;
> struct sk_buff *skb;
> - int quote, cnt, tmp;
> + int quote, *cnt, tmp;
>
> BT_DBG("%s", hdev->name);
>
> @@ -3970,9 +3970,9 @@ static void hci_sched_le(struct hci_dev *hdev)
> hci_link_tx_to(hdev, LE_LINK);
> }
>
> - cnt = hdev->le_pkts ? hdev->le_cnt : hdev->acl_cnt;
> - tmp = cnt;
> - while (cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
> + cnt = hdev->le_pkts ? &hdev->le_cnt : &hdev->acl_cnt;
> + tmp = *cnt;
> + while (*cnt && (chan = hci_chan_sent(hdev, LE_LINK, "e))) {
> u32 priority = (skb_peek(&chan->data_q))->priority;
> while (quote-- && (skb = skb_peek(&chan->data_q))) {
> BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
> @@ -3987,18 +3987,13 @@ static void hci_sched_le(struct hci_dev *hdev)
> hci_send_frame(hdev, skb);
> hdev->le_last_tx = jiffies;
>
> - cnt--;
> + *cnt--;
This needs to be (*cnt)--; instead.
> chan->sent++;
> chan->conn->sent++;
> }
> }
>
> - if (hdev->le_pkts)
> - hdev->le_cnt = cnt;
> - else
> - hdev->acl_cnt = cnt;
> -
> - if (cnt != tmp)
> + if (*cnt != tmp)
> hci_prio_recalculate(hdev, LE_LINK);
> }
Tested with my suggested change and it fixes the original issue. Mind
sending the updated patch instead then?
Thanks!
--
Ricardo Salveti
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
