| Message ID | 20200821074512.19985-1-sonnysasaka@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ] device: Fix race condition between device connection and disconnection | expand |
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.
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
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. + 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;
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(+)