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;
MIME-Version: 1.0
References: <20220722182248.1.I20e96c839200bb75cd6af80384f16c8c01498f57@changeid>
 <CABBYNZ+k+ZOVNi+GYgdiK+B+tqKQXTWYp1QG4JSE_7EwZHNBMw@mail.gmail.com>
In-Reply-To: <CABBYNZ+k+ZOVNi+GYgdiK+B+tqKQXTWYp1QG4JSE_7EwZHNBMw@mail.gmail.com>
From:   Archie Pusaka <apusaka@google.com>
Date:   Mon, 25 Jul 2022 18:09:44 +0800
Message-ID: <CAJQfnxFdnjHdPTdfgE+v=ero01H0dE6LsJ9z3K5LCZ28gogYtw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: hci_sync: Use safe loop when adding accept list
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Zhengping Jiang <jiangzp@google.com>,
        Michael Sun <michaelfsun@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Eric and Luiz,

>  "the userspace can still remove devices" is a bit vague.
I mean removing devices via MGMT command.

> It seems that the issue at hand is that hci_le_add_accept_list_sync() can
> move the current item from  pend_le_conns / pend_le_reports lists ?
The issue is, hci_le_add_accept_list_sync() is iterating the lists
when the content is being removed elsewhere.

> Hopefully these lists can not be changed by other threads while
> hci_update_accept_list_sync() is running ?
Probably. Looks like Luiz also thinks the same way.

> Please add a Fixes: tag
Unfortunately I don't know when this is introduced.

> Hmm if this happens it means other threads are actually interfering
> with cmd_sync queue which is something that is probably a bug since
> the whole point of cmd_sync is to serialize the commands making it
> easier to do more complex state updates (such accept+resolve list
> updates)
Thanks, I haven't fully grasped the intention of having hci_sync and
how to properly use it.

> we could perhaps still apply this change as a workaround but
> ultimately I think it would be better to add a mgmt-tester reproducing
> the issue and have a proper fix of the code updating the list from a
> different thread.
Agree. Having said that, I don't think currently I have the time to
invest in writing a test and a proper fix, so my apologies on this.

Best,
Archie