Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4263c3ba-036b-ae67-ab29-166325ce6aba@samsung.com>
Date:   Thu, 8 Sep 2022 12:25:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.13.0
Subject: Re: [PATCH v7 2/7] Bluetooth: Add initial implementation of CIS
 connections
Content-Language: en-US
From:   Marek Szyprowski <m.szyprowski@samsung.com>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
In-Reply-To: <41717010-b7a5-40e4-39d7-722fb6d18eae@samsung.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20220711233535.3134546-1-luiz.dentz@gmail.com>
        <CGME20220809085500eucas1p18cb2752164ca55832787e7d3843f48ab@eucas1p1.samsung.com>
        <20220711233535.3134546-2-luiz.dentz@gmail.com>
        <749ed578-bc9d-b699-3602-bee444fff9bb@samsung.com>
        <CABBYNZLsR0KT1-OVVjnUzLp0+U0eHGi0PNqKbfntp_5AHNDawA@mail.gmail.com>
        <03f9bea9-77fb-2b57-43d0-c4b819c2560a@samsung.com>
        <CABBYNZ+vRqqCBVTgQJ-UZ+gd2ceFebQF94NDdYcN4Xmyk+GWTA@mail.gmail.com>
        <92a47171-f69b-5bf9-6e8c-70fc3e9e6eee@samsung.com>
        <41717010-b7a5-40e4-39d7-722fb6d18eae@samsung.com>
Precedence: bulk

Hi All,

On 16.08.2022 17:24, Marek Szyprowski wrote:
> On 12.08.2022 14:33, Marek Szyprowski wrote:
>> On 10.08.2022 22:04, Luiz Augusto von Dentz wrote:
>>> On Wed, Aug 10, 2022 at 7:18 AM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 09.08.2022 21:24, Luiz Augusto von Dentz wrote:
>>>>> On Tue, Aug 9, 2022 at 1:55 AM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com>  wrote:
>>>>>> On 12.07.2022 01:35, Luiz Augusto von Dentz wrote:
>>>>>>> From: Luiz Augusto von Dentz<luiz.von.dentz@intel.com>
>>>>>>>
>>>>>>> This adds the initial implementation of CIS connections and 
>>>>>>> introduces
>>>>>>> the ISO packets/links.
>>>>>>>
>>>>>>> == Central: Set CIG Parameters, create a CIS and Setup Data Path ==
>>>>>>>
>>>>>>>> tools/isotest -s <address>
>>>>>>> < HCI Command: LE Extended Create... (0x08|0x0043) plen 26
>>>>>>> ...
>>>>>>>> HCI Event: Command Status (0x0f) plen 4
>>>>>>>          LE Extended Create Connection (0x08|0x0043) ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>> HCI Event: LE Meta Event (0x3e) plen 31
>>>>>>>          LE Enhanced Connection Complete (0x0a)
>>>>>>>          ...
>>>>>>> < HCI Command: LE Create Connected... (0x08|0x0064) plen 5
>>>>>>> ...
>>>>>>>> HCI Event: Command Status (0x0f) plen 4
>>>>>>>          LE Create Connected Isochronous Stream (0x08|0x0064) 
>>>>>>> ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>> HCI Event: LE Meta Event (0x3e) plen 29
>>>>>>>          LE Connected Isochronous Stream Established (0x19)
>>>>>>>          ...
>>>>>>> < HCI Command: LE Setup Isochronou.. (0x08|0x006e) plen 13
>>>>>>> ...
>>>>>>>> HCI Event: Command Complete (0x0e) plen 6
>>>>>>>          LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>            Handle: 257
>>>>>>> < HCI Command: LE Setup Isochronou.. (0x08|0x006e) plen 13
>>>>>>> ...
>>>>>>>> HCI Event: Command Complete (0x0e) plen 6
>>>>>>>          LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>            Handle: 257
>>>>>>>
>>>>>>> == Peripheral: Accept CIS and Setup Data Path ==
>>>>>>>
>>>>>>>> tools/isotest -d
>>>>>>>     HCI Event: LE Meta Event (0x3e) plen 7
>>>>>>>          LE Connected Isochronous Stream Request (0x1a)
>>>>>>> ...
>>>>>>> < HCI Command: LE Accept Co.. (0x08|0x0066) plen 2
>>>>>>> ...
>>>>>>>> HCI Event: LE Meta Event (0x3e) plen 29
>>>>>>>          LE Connected Isochronous Stream Established (0x19)
>>>>>>> ...
>>>>>>> < HCI Command: LE Setup Is.. (0x08|0x006e) plen 13
>>>>>>> ...
>>>>>>>> HCI Event: Command Complete (0x0e) plen 6
>>>>>>>          LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>            Handle: 257
>>>>>>> < HCI Command: LE Setup Is.. (0x08|0x006e) plen 13
>>>>>>> ...
>>>>>>>> HCI Event: Command Complete (0x0e) plen 6
>>>>>>>          LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1
>>>>>>>            Status: Success (0x00)
>>>>>>>            Handle: 257
>>>>>>>
>>>>>>> Signed-off-by: Luiz Augusto von Dentz<luiz.von.dentz@intel.com>
>>>>>> This patch landed recently in linux-next as commit 26afbd826ee3
>>>>>> ("Bluetooth: Add initial implementation of CIS connections").
>>>>>> Unfortunately it causes a regression on my test systems. On 
>>>>>> almost all
>>>>>> I've observed that calling a simple 'hcitool scan' command in a 
>>>>>> shell
>>>>>> fails in an unexpected way:
>>>>>>
>>>>>> $ hcitool scan
>>>>>> *** stack smashing detected ***: <unknown> terminated
>>>>>> Aborted
>>>>> Not really sure how it would be related to ISO changes though, have
>>>>> you even enabled ISO sockets UUID? Perhaps check if there is 
>>>>> something
>>>>> on dmesg indicating what is going on since I tried here and that
>>>>> doesn't seem to cause any problem:
>>>>>
>>>>> tools/hcitool scan
>>>>> Scanning ...
>>>>>
>>>>> Perhaps it is a combination of using old userspace tools with new
>>>>> kernel, but then again these changes should affect something like
>>>>> hcitool.
>>>> Indeed my userspace is old, but still, the kernel changes shouldn't 
>>>> make
>>>> it to crash. I didn't change anything in userspace since ages, so I
>>>> assume that ISO sockets UUIDs are not enabled. Maybe it is somehow
>>>> architecture related or specific? It looks that only ARM 32bit 
>>>> userspace
>>>> apps crashes. I've just checked 64bit userspace on ARM64 (RPi4) and it
>>>> works fine with that commit.
>>> That would be the first time it happens to me that a change in kernel
>>> would crash the userspace in such odd fashion, btw perhaps run with
>>> valgrind so it generates a backtrace of where it would be crashing,
>>> well if that is reproducible with valgrind.
>>
>> Well, its not that easy. I've checked and Debian doesn't provide a 
>> valgrind package for the buster/armel arch, which I use on my test 
>> systems (for some historical reasons). Building everything from the 
>> source will take some time, though. I will try to do this after 
>> getting back from my holidays after 24th Aug.
>
> Unfortunately my holidays trip didn't start, so I had a chance to play 
> a bit with this issue.
>
> Valgrind doesn't really provide any useful information here:
>
> $ valgrind hcitool scan
> ==1391== Memcheck, a memory error detector
> ==1391== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==1391== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright 
> info
> ==1391== Command: hcitool scan
> ==1391==
> *** stack smashing detected ***: <unknown> terminated
> ==1391==
> ==1391== Process terminating with default action of signal 6 (SIGABRT)
> ==1391==    at 0x48EB6AC: raise (raise.c:51)
> ==1391==    by 0x48D6283: abort (abort.c:79)
> ==1391==    by 0x4928E3B: __libc_message (libc_fatal.c:181)
> ==1391==    by 0x49ACFE3: __fortify_fail_abort (fortify_fail.c:33)
> ==1391==    by 0x49ACFA7: __stack_chk_fail (stack_chk_fail.c:29)
> ==1391==    by 0x117DFB: ??? (in /usr/bin/hcitool)
> ==1391==
> ==1391== HEAP SUMMARY:
> ==1391==     in use at exit: 132 bytes in 1 blocks
> ==1391==   total heap usage: 1 allocs, 0 frees, 132 bytes allocated
> ==1391==
> ==1391== LEAK SUMMARY:
> ==1391==    definitely lost: 0 bytes in 0 blocks
> ==1391==    indirectly lost: 0 bytes in 0 blocks
> ==1391==      possibly lost: 0 bytes in 0 blocks
> ==1391==    still reachable: 132 bytes in 1 blocks
> ==1391==         suppressed: 0 bytes in 0 blocks
> ==1391== Rerun with --leak-check=full to see details of leaked memory
> ==1391==
> ==1391== For counts of detected and suppressed errors, rerun with: -v
> ==1391== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
> Aborted
>
>
> I've also checked the ARM 32bit 'armhf' userspace abi on vanilla 
> Raspberry Pi OS Lite 32-bit image (from April 4th 2022). The issue is 
> same:
>
> $ hcitool scan
> *** stack smashing detected ***: terminated
> Aborted
>
> It is enough to boot that image with init=/bin/bash and run 'hcitool 
> scan' to reproduce the issue...
>

I've had some time to analyze this issue further and I've finally found 
which change is responsible for this issue. It is caused by the 
following chunk:

diff --git a/include/net/bluetooth/hci_sock.h 
b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..0520e21ab698 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h
@@ -124,6 +124,8 @@ struct hci_dev_info {
         __u16 acl_pkts;
         __u16 sco_mtu;
         __u16 sco_pkts;
+       __u16 iso_mtu;
+       __u16 iso_pkts;

         struct hci_dev_stats stat;
  };

It looks that this is an ABI break for some old userspace tools. I've 
confirmed this by applying only the above chunk on top of v5.19-rc1 and 
running my tests. 'hcitool scan' crashes in such case.

I've also removed that chunk from the v6.0-rc1 release and surprisingly 
I found that it is not used by the bluetooth code! Kernel compiled 
successfully. Is that change intentional? Or is it just a leftover from 
the development process that accidentally made its way into final patch?


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland