2021-06-10 18:20:17

by Sonny Sasaka

[permalink] [raw]
Subject: [PATCH BlueZ] Check whether device is connected before attaching EATT

Due to a race condition, device_attach_att() may be reached when the
dev is actually already disconnected but dev->att is not yet cleaned up
by att_disconnect_cb(). Therefore we should check whether the dev is
connected before attaching EATT.

The race condition is discovered at rare cases when there is a very
quick reconnection after disconnection so that device_attach_att() is
called even before att_disconnect_cb(). This case is more probable to
happen when the host goes to suspend right before dev_disconnected() is
invoked and when the host is woken up by a reconnection the reconnection
is processed earlier than the cleanup in att_disconnect_cb().

Reviewed-by: Abhishek Pandit-Subedi <[email protected]>

---
src/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/device.c b/src/device.c
index 65838f59f..319a929ee 100644
--- a/src/device.c
+++ b/src/device.c
@@ -5306,7 +5306,7 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io)
return false;
}

- if (dev->att) {
+ if (btd_device_is_connected(dev) && dev->att) {
if (btd_opts.gatt_channels == bt_att_get_channels(dev->att)) {
DBG("EATT channel limit reached");
return false;
--
2.31.0


2021-06-10 19:04:38

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ] Check whether device is connected before attaching EATT

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=498303

---Test result---

Test Summary:
CheckPatch PASS 0.57 seconds
GitLint PASS 0.12 seconds
Prep - Setup ELL PASS 46.51 seconds
Build - Prep PASS 0.14 seconds
Build - Configure PASS 8.12 seconds
Build - Make PASS 204.53 seconds
Make Check PASS 9.47 seconds
Make Distcheck PASS 240.18 seconds
Build w/ext ELL - Configure PASS 8.21 seconds
Build w/ext ELL - Make PASS 192.78 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth

2021-06-10 21:03:01

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ] Check whether device is connected before attaching EATT

Hi Sonny,

On Thu, Jun 10, 2021 at 11:20 AM Sonny Sasaka <[email protected]> wrote:
>
> Due to a race condition, device_attach_att() may be reached when the
> dev is actually already disconnected but dev->att is not yet cleaned up
> by att_disconnect_cb(). Therefore we should check whether the dev is
> connected before attaching EATT.
>
> The race condition is discovered at rare cases when there is a very
> quick reconnection after disconnection so that device_attach_att() is
> called even before att_disconnect_cb(). This case is more probable to
> happen when the host goes to suspend right before dev_disconnected() is
> invoked and when the host is woken up by a reconnection the reconnection
> is processed earlier than the cleanup in att_disconnect_cb().
>
> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
>
> ---
> src/device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/device.c b/src/device.c
> index 65838f59f..319a929ee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -5306,7 +5306,7 @@ bool device_attach_att(struct btd_device *dev, GIOChannel *io)
> return false;
> }
>
> - if (dev->att) {
> + if (btd_device_is_connected(dev) && dev->att) {
> if (btd_opts.gatt_channels == bt_att_get_channels(dev->att)) {
> DBG("EATT channel limit reached");
> return false;

Perhaps we should have this check earlier, also there seems to be
something wrong with att_io then, if the device is no longer connected
att_io shall have been unrefed as well.

> --
> 2.31.0
>


--
Luiz Augusto von Dentz