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: <20200821074512.19985-1-sonnysasaka@chromium.org> <CABBYNZJp2cCqRFa2zrdLnva0UgxvG2eBhYyKr52Rx54sU=znSA@mail.gmail.com>
In-Reply-To: <CABBYNZJp2cCqRFa2zrdLnva0UgxvG2eBhYyKr52Rx54sU=znSA@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Fri, 21 Aug 2020 11:01:25 -0700
Message-ID: <CAO271m=ZwWs4oKH1xBRD9BwxOdiA05bLgzsA-++gTzQwEv05jA@mail.gmail.com>
Subject: Re: [PATCH BlueZ] device: Fix race condition between device
 connection and disconnection
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

Thanks for the feedback. I have sent a v2 fixing the comment style.

On Fri, Aug 21, 2020 at 10:36 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Fri, Aug 21, 2020 at 12:47 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > When Connect() is called and waiting for return, dev_disconnected may be
> > called due to MGMT_EV_DEVICE_DISCONNECTED event from kernel. In that
> > case reply to client that the connection failed otherwise the dbus
> > method will timeout because bluetoothd never replies.
> >
> > Tested with simulation of a lot of Connect() to bluetooth devices and
> > check that error is returned from bluetoothd rather than dbus timeout
> > when this race condition happens.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> >
> > ---
> >  src/device.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/device.c b/src/device.c
> > index 7b7808405..55e7b4042 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -3006,6 +3006,7 @@ void device_add_connection(struct btd_device *dev, uint8_t bdaddr_type)
> >  void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> >  {
> >         struct bearer_state *state = get_state(device, bdaddr_type);
> > +       DBusMessage *reply;
> >
> >         if (!state->connected)
> >                 return;
> > @@ -3020,6 +3021,17 @@ void device_remove_connection(struct btd_device *device, uint8_t bdaddr_type)
> >                 device->disconn_timer = 0;
> >         }
> >
> > +       // This could be executed while the client is waiting for Connect() but
> > +       // att_connect_cb has not been invoked.
> > +       // In that case reply the client that the connection failed.
>
> Please use C style comment /* */
>
> > +       if (device->connect) {
> > +               DBG("connection removed while Connect() is waiting reply");
> > +               reply = btd_error_failed(device->connect, "Disconnected early");
> > +               g_dbus_send_message(dbus_conn, reply);
> > +               dbus_message_unref(device->connect);
> > +               device->connect = NULL;
> > +       }
> > +
> >         while (device->disconnects) {
> >                 DBusMessage *msg = device->disconnects->data;
> >
> > --
> > 2.26.2
> >
>
> Otherwise it looks good.
>
> --
> Luiz Augusto von Dentz