Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200608180241.BlueZ.v1.1.Ibf8331f6c835d53fe7ca978de962f93981573d9a@changeid>
 <CACumGOKqrBQJzYt_ayW6KRmu9QKCCPYiojdozvGwO3yTR-2Jyg@mail.gmail.com>
 <CACumGOKyAfdgP6t4PnNBzmVmFayV4b3gPOjux3aGg_de2T104g@mail.gmail.com> <CABmPvSE_wwju1pe7tZoZHywF7oAz8OvJCbebE4gZOe5OHVaLpg@mail.gmail.com>
In-Reply-To: <CABmPvSE_wwju1pe7tZoZHywF7oAz8OvJCbebE4gZOe5OHVaLpg@mail.gmail.com>
From:   "Von Dentz, Luiz" <luiz.von.dentz@intel.com>
Date:   Thu, 11 Jun 2020 10:31:06 -0700
Message-ID: <CACumGO+iuMkMhZkfP7DyFvXSDyU-M2RMVc7cN1-pfHTh95CyUg@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1] adapter: Fix the unref and reset of
 watch_client's members
To:     Miao-chen Chou <mcchou@chromium.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Michael Sun <michaelfsun@google.com>,
        Alain Michaud <alainm@chromium.org>,
        Yoni Shavit <yshavit@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Miao,

On Wed, Jun 10, 2020 at 3:18 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> For 0001-adapter-Do-not-remove-client-watch-directly-if-disco.patch,
> it looks good to me.
> For 0002-adapter-Consolitate-code-for-discovery-reply.patch, please
> see me following comments.
>
> +static void discovery_reply(struct watch_client *client, uint8_t status)
> +{
> +       DBusMessage *reply;
> +
> +       if (!client->msg)
> +               return;
> +
> +       if (!status) {
> It'd better to change this to "if (status == MGMT_STATUS_SUCCESS) {".
> +               g_dbus_send_reply(dbus_conn, client->msg, DBUS_TYPE_INVALID);
> +       } else  {
> +               reply = btd_error_busy(client->msg);
> +               g_dbus_send_message(dbus_conn, reply);
> +       }
> +
> +       dbus_message_unref(client->msg);
> +       client->msg = NULL;
> +}
> I also notice that we treated the status other than
> MGMT_STATUS_SUCCESS to be busy, but this can be addressed as a
> separate patch.

Wasn't that the error we were generating before? Afaik both start and
stop discovery were doing the same in this regard.

> For 0003-adapter-Fix-possible-crash-when-stopping-discovery.patch, I
> had few comments here.
> (1) I didn't see the corresponding changes to pass the pointer of the
> adapter as the user data when sending MGMT_OP_STOP_DISCOVERY command.
> Should it be part of the patch?

Yep, it should be fixed now.

> (2) This does resolve the crashing due to use-after-free of a
> watch_client. However, the following logic doesn't seem to be correct
> to me. If you recall the call path that we discussed, which is
> "client1 start_discovery() -> client1 start_discovery_complete() ->
> client1 stop_discovery() -> client2 start_discovery() -> client1
> detach from D-Bus which triggers discovery_disconnect() -> client1
> stop_discovery_complete() -> crash)",
> when client2 starts the discovery, client2 is added to
> adapter->discovery_list, so once stop_discovery_complete() is called
> with client1, client2 is the only client in adapter->discovery_list.
> And this statement remains true even with this patch. That being said,
> the following "client = adapter->discovery_list->data" would return
> client2, which is not supposed to be replied by
> stop_discovery_complete() issued by client1. Agree?

Yep, that will need tracking of the client who initiated the
operation, I will send a patch for that later today.

> +       if (!adapter->discovery_list)
> +               return;
> +
> +       client = adapter->discovery_list->data;
>
> Thanks,
> Miao
>
> On Tue, Jun 9, 2020 at 2:25 PM Von Dentz, Luiz <luiz.von.dentz@intel.com> wrote:
> >
> > Hi,
> >
> >
> > On Mon, Jun 8, 2020 at 6:11 PM Von Dentz, Luiz <luiz.von.dentz@intel.com> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Mon, Jun 8, 2020 at 6:03 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >>
> > >> This properly handles the unref of client->msg in
> > >> stop_discovery_complete() and the reset of it. This also handles the unref
> > >> of client->msg, the reset of client->watch and the reset of client->msg in
> > >> start_discovery_complete().
> > >>
> > >> The following test was performed:
> > >> (1) Intentionally changed the MGMT status other than MGMT_STATUS_SUCCESS
> > >> in stop_discovery_complete() and start_discovery_complete() and built
> > >> bluetoothd.
> > >> (2) In bluetoothctl console, issued scan on/scan off to invoke
> > >> StartDiscovery and verified that new discovery requests can be processed.
> > >>
> > >> Reviewed-by: Alain Michaud <alainm@chromium.org>
> > >> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > >> ---
> > >>
> > >>  src/adapter.c | 5 +++++
> > >>  1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/src/adapter.c b/src/adapter.c
> > >> index 76acfea70..0857a3115 100644
> > >> --- a/src/adapter.c
> > >> +++ b/src/adapter.c
> > >> @@ -1652,6 +1652,9 @@ fail:
> > >>                 reply = btd_error_busy(client->msg);
> > >>                 g_dbus_send_message(dbus_conn, reply);
> > >>                 g_dbus_remove_watch(dbus_conn, client->watch);
> > >
> > >
> > > We shouldn't be removing the watch directly since the client may have registered filters so we let discovery_remove do it by calling discovery_free if necessary.
> > >
> > >>
> > >> +               client->watch = 0;
> > >> +               dbus_message_unref(client->msg);
> > >> +               client->msg = NULL;
> > >>                 discovery_remove(client, false);
> > >>                 return;
> > >>         }
> > >> @@ -1926,6 +1929,8 @@ static void stop_discovery_complete(uint8_t status, uint16_t length,
> > >>                 if (client->msg) {
> > >>                         reply = btd_error_busy(client->msg);
> > >>                         g_dbus_send_message(dbus_conn, reply);
> > >> +                       dbus_message_unref(client->msg);
> > >> +                       client->msg = NULL;
> > >>                 }
> > >>                 goto done;
> > >>         }
> > >> --
> > >> 2.26.2
> > >
> > >
> > > Ive sent similar fixes upstream, let me attach them here just in case.
> >
> > Any comments on these changes, I would like to push them as soon as possible.