[PATCH BlueZ 0/2] AVDTP SEID pool seems too small

[PATCH BlueZ 0/2] AVDTP SEID pool seems too small

[Pauli Virtanen]

Hi,

It seems that if media application is registered to multiple BT
adapters, BlueZ can easily run out of free AVDTP SEIDs.  This occurs in
practice when running Pipewire (or Pulseaudio) and plugging in 3+
adapters: their media application has 10+ SEPs for various vendor A2DP
codecs, registered for each adapter.  After having registered 32 SEPs
across all adapters, Media1.RegisterApplication starts failing, due to
SEP registration not finding free SEIDs.

However, if I understand the AVDTP spec and BlueZ implementation
correctly, this behavior is not quite correct: (i) the SEID pool in
BlueZ is limited to 8*sizeof(int) == 32 entries, which is smaller than
the 0x3E = 62 entries (AVDTP1.3, 8.20.1).  Also, (ii) BlueZ allocates
SEIDs from a pool shared by all adapters, whereas it seems they should
be device & connection local (AVDTP1.3, 4.10). Since adapters are
separate devices in this context, this then appears to imply that each
should have separate SEID pool.

If this interpretation is right (I don't know the AVDTP spec very
well, so...), here are two patches that address this.  In (limited)
tests this seemed to work.

Pauli Virtanen (2):
  shared/util: use 64-bit bitmap in util_get/clear_uid
  avdtp: use separate local SEID pool for each adapter

 android/avdtp.c        |  2 +-
 profiles/audio/a2dp.c  |  2 +-
 profiles/audio/avdtp.c | 55 ++++++++++++++++++++++++++++++++++++------
 profiles/audio/avdtp.h |  4 ++-
 src/advertising.c      |  2 +-
 src/shared/util.c      | 27 ++++++++++++---------
 src/shared/util.h      |  4 +--
 unit/test-avdtp.c      |  2 +-
 8 files changed, 71 insertions(+), 27 deletions(-)

-- 
2.31.1

[PATCH BlueZ 1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[Pauli Virtanen]

The util_get/clear_uid functions use int type for bitmap, and are used
e.g. for SEID allocation. However, valid SEIDs are in range 1 to 0x3E
(AVDTP spec v1.3, 8.20.1), and 8*sizeof(int) is often smaller than 0x3E.

The function is also used in src/advertising.c, but an explicit maximum
value is always provided, so growing the bitmap size is safe there.

Use 64-bit bitmap instead, to be able to cover the valid range.
---
 android/avdtp.c        |  2 +-
 profiles/audio/avdtp.c |  2 +-
 src/advertising.c      |  2 +-
 src/shared/util.c      | 27 +++++++++++++++------------
 src/shared/util.h      |  4 ++--
 unit/test-avdtp.c      |  2 +-
 6 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index 8c2930ec1..a261a8e5f 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -34,7 +34,7 @@
 #include "../profiles/audio/a2dp-codecs.h"
 
 #define MAX_SEID 0x3E
-static unsigned int seids;
+static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 946231b71..25520ceec 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -44,7 +44,7 @@
 #define AVDTP_PSM 25
 
 #define MAX_SEID 0x3E
-static unsigned int seids;
+static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
diff --git a/src/advertising.c b/src/advertising.c
index bd79454d5..41b818650 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -48,7 +48,7 @@ struct btd_adv_manager {
 	uint8_t max_scan_rsp_len;
 	uint8_t max_ads;
 	uint32_t supported_flags;
-	unsigned int instance_bitmap;
+	uint64_t instance_bitmap;
 	bool extended_add_cmds;
 	int8_t min_tx_power;
 	int8_t max_tx_power;
diff --git a/src/shared/util.c b/src/shared/util.c
index 244756456..723dedd75 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -124,30 +124,33 @@ unsigned char util_get_dt(const char *parent, const char *name)
 
 /* Helpers for bitfield operations */
 
-/* Find unique id in range from 1 to max but no bigger then
- * sizeof(int) * 8. ffs() is used since it is POSIX standard
- */
-uint8_t util_get_uid(unsigned int *bitmap, uint8_t max)
+/* Find unique id in range from 1 to max but no bigger than 64. */
+uint8_t util_get_uid(uint64_t *bitmap, uint8_t max)
 {
 	uint8_t id;
 
-	id = ffs(~*bitmap);
+	if (max > 64)
+		max = 64;
 
-	if (!id || id > max)
-		return 0;
+	for (id = 1; id <= max; ++id) {
+		uint64_t mask = ((uint64_t)1) << (id - 1);
 
-	*bitmap |= 1u << (id - 1);
+		if (!(*bitmap & mask)) {
+			*bitmap |= mask;
+			return id;
+		}
+	}
 
-	return id;
+	return 0;
 }
 
 /* Clear id bit in bitmap */
-void util_clear_uid(unsigned int *bitmap, uint8_t id)
+void util_clear_uid(uint64_t *bitmap, uint8_t id)
 {
-	if (!id)
+	if (id == 0 || id > 64)
 		return;
 
-	*bitmap &= ~(1u << (id - 1));
+	*bitmap &= ~(((uint64_t)1) << (id - 1));
 }
 
 static const struct {
diff --git a/src/shared/util.h b/src/shared/util.h
index 9920b7f76..60908371d 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -102,8 +102,8 @@ void util_hexdump(const char dir, const unsigned char *buf, size_t len,
 
 unsigned char util_get_dt(const char *parent, const char *name);
 
-uint8_t util_get_uid(unsigned int *bitmap, uint8_t max);
-void util_clear_uid(unsigned int *bitmap, uint8_t id);
+uint8_t util_get_uid(uint64_t *bitmap, uint8_t max);
+void util_clear_uid(uint64_t *bitmap, uint8_t id);
 
 const char *bt_uuid16_to_str(uint16_t uuid);
 const char *bt_uuid32_to_str(uint32_t uuid);
diff --git a/unit/test-avdtp.c b/unit/test-avdtp.c
index f5340d6f3..4e8a68c6b 100644
--- a/unit/test-avdtp.c
+++ b/unit/test-avdtp.c
@@ -550,7 +550,7 @@ static void test_server_seid(gconstpointer data)
 	struct avdtp_local_sep *sep;
 	unsigned int i;
 
-	for (i = 0; i < sizeof(int) * 8; i++) {
+	for (i = 0; i < MAX_SEID; i++) {
 		sep = avdtp_register_sep(context->lseps, AVDTP_SEP_TYPE_SINK,
 						AVDTP_MEDIA_TYPE_AUDIO,
 						0x00, TRUE, &sep_ind, NULL,
-- 
2.31.1

[PATCH BlueZ 2/2] avdtp: use separate local SEID pool for each adapter

[Pauli Virtanen]

Local SEIDs are currently allocated from a pool that is common for all
adapters. However, AVDTP spec v1.3, sec 4.10 states "To prevent
conflicts, the scope of the SEID shall be both device-local and
connection-local. The application is responsible for assigning a SEID,
which is not in use on the connection to the same peer device." In
practice, registering the same media application for multiple adapters
can result to running out of SEIDs, even though the spec does not
require SEIDs to be unique across adapters.

Use a separate SEID pool for each btd_adapter to fix this.
---
 profiles/audio/a2dp.c  |  2 +-
 profiles/audio/avdtp.c | 55 ++++++++++++++++++++++++++++++++++++------
 profiles/audio/avdtp.h |  4 ++-
 3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 02caa83e1..1e8a66b8a 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -2615,7 +2615,7 @@ struct a2dp_sep *a2dp_add_sep(struct btd_adapter *adapter, uint8_t type,
 
 	sep = g_new0(struct a2dp_sep, 1);
 
-	sep->lsep = avdtp_register_sep(server->seps, type,
+	sep->lsep = avdtp_register_sep(adapter, server->seps, type,
 					AVDTP_MEDIA_TYPE_AUDIO, codec,
 					delay_reporting, &endpoint_ind,
 					&cfm, sep);
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 25520ceec..f2aa98b23 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -44,7 +44,6 @@
 #define AVDTP_PSM 25
 
 #define MAX_SEID 0x3E
-static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
@@ -325,6 +324,7 @@ struct avdtp_local_sep {
 	GSList *caps;
 	struct avdtp_sep_ind *ind;
 	struct avdtp_sep_cfm *cfm;
+	struct btd_adapter *adapter;
 	void *user_data;
 };
 
@@ -414,6 +414,8 @@ struct avdtp {
 
 static GSList *state_callbacks = NULL;
 
+static GHashTable *adapter_seids = NULL;
+
 static int send_request(struct avdtp *session, gboolean priority,
 			struct avdtp_stream *stream, uint8_t signal_id,
 			void *buffer, size_t size);
@@ -3768,7 +3770,41 @@ int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
 							&req, sizeof(req));
 }
 
-struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
+static uint8_t get_adapter_seid(struct btd_adapter *adapter)
+{
+	uint64_t *seids;
+
+	if (adapter_seids == NULL)
+		adapter_seids = g_hash_table_new_full(g_direct_hash,
+						g_direct_equal, NULL, g_free);
+
+	seids = g_hash_table_lookup(adapter_seids, adapter);
+
+	if (seids == NULL) {
+		seids = g_new0(uint64_t, 1);
+		g_hash_table_insert(adapter_seids, adapter, seids);
+	}
+
+	return util_get_uid(seids, MAX_SEID);
+}
+
+static void clear_adapter_seid(struct btd_adapter *adapter, uint8_t seid)
+{
+	uint64_t *seids = adapter_seids ?
+			g_hash_table_lookup(adapter_seids, adapter) : NULL;
+
+	if (seids == NULL)
+		return;
+
+	util_clear_uid(seids, seid);
+
+	if (*seids == 0)
+		g_hash_table_remove(adapter_seids, adapter);
+}
+
+struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
+						struct queue *lseps,
+						uint8_t type,
 						uint8_t media_type,
 						uint8_t codec_type,
 						gboolean delay_reporting,
@@ -3777,7 +3813,7 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
 						void *user_data)
 {
 	struct avdtp_local_sep *sep;
-	uint8_t seid = util_get_uid(&seids, MAX_SEID);
+	uint8_t seid = get_adapter_seid(adapter);
 
 	if (!seid)
 		return NULL;
@@ -3791,11 +3827,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
 	sep->codec = codec_type;
 	sep->ind = ind;
 	sep->cfm = cfm;
+	sep->adapter = adapter;
 	sep->user_data = user_data;
 	sep->delay_reporting = delay_reporting;
 
-	DBG("SEP %p registered: type:%d codec:%d seid:%d", sep,
-			sep->info.type, sep->codec, sep->info.seid);
+	DBG("SEP %p registered: type:%d codec:%d adapter:%p seid:%d", sep,
+			sep->info.type, sep->codec, sep->adapter,
+			sep->info.seid);
 
 	if (!queue_push_tail(lseps, sep)) {
 		g_free(sep);
@@ -3813,10 +3851,11 @@ int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep)
 	if (sep->stream)
 		release_stream(sep->stream, sep->stream->session);
 
-	DBG("SEP %p unregistered: type:%d codec:%d seid:%d", sep,
-			sep->info.type, sep->codec, sep->info.seid);
+	DBG("SEP %p unregistered: type:%d codec:%d adapter:%p seid:%d", sep,
+			sep->info.type, sep->codec, sep->adapter,
+			sep->info.seid);
 
-	util_clear_uid(&seids, sep->info.seid);
+	clear_adapter_seid(sep->adapter, sep->info.seid);
 	queue_remove(lseps, sep);
 	g_free(sep);
 
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index b02534cd5..70807cff9 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -278,7 +278,9 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream);
 int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
 							uint16_t delay);
 
-struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
+struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
+						struct queue *lseps,
+						uint8_t type,
 						uint8_t media_type,
 						uint8_t codec_type,
 						gboolean delay_reporting,
-- 
2.31.1

RE: AVDTP SEID pool seems too small

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

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=538885

---Test result---

Test Summary:
CheckPatch                    FAIL      0.56 seconds
GitLint                       PASS      0.20 seconds
Prep - Setup ELL              PASS      39.86 seconds
Build - Prep                  PASS      0.09 seconds
Build - Configure             PASS      7.00 seconds
Build - Make                  PASS      173.79 seconds
Make Check                    PASS      8.45 seconds
Make Distcheck                PASS      207.44 seconds
Build w/ext ELL - Configure   PASS      7.17 seconds
Build w/ext ELL - Make        PASS      165.23 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
avdtp: use separate local SEID pool for each adapter
ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#54: FILE: profiles/audio/avdtp.c:417:
+static GHashTable *adapter_seids = NULL;

- total: 1 errors, 0 warnings, 119 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] avdtp: use separate local SEID pool for each adapter" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
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

Re: [PATCH BlueZ 2/2] avdtp: use separate local SEID pool for each adapter

[Luiz Augusto von Dentz]

Hi Pauli,

On Sun, Aug 29, 2021 at 8:52 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Local SEIDs are currently allocated from a pool that is common for all
> adapters. However, AVDTP spec v1.3, sec 4.10 states "To prevent
> conflicts, the scope of the SEID shall be both device-local and
> connection-local. The application is responsible for assigning a SEID,
> which is not in use on the connection to the same peer device." In
> practice, registering the same media application for multiple adapters
> can result to running out of SEIDs, even though the spec does not
> require SEIDs to be unique across adapters.
>
> Use a separate SEID pool for each btd_adapter to fix this.
> ---
>  profiles/audio/a2dp.c  |  2 +-
>  profiles/audio/avdtp.c | 55 ++++++++++++++++++++++++++++++++++++------
>  profiles/audio/avdtp.h |  4 ++-
>  3 files changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> index 02caa83e1..1e8a66b8a 100644
> --- a/profiles/audio/a2dp.c
> +++ b/profiles/audio/a2dp.c
> @@ -2615,7 +2615,7 @@ struct a2dp_sep *a2dp_add_sep(struct btd_adapter *adapter, uint8_t type,
>
>         sep = g_new0(struct a2dp_sep, 1);
>
> -       sep->lsep = avdtp_register_sep(server->seps, type,
> +       sep->lsep = avdtp_register_sep(adapter, server->seps, type,
>                                         AVDTP_MEDIA_TYPE_AUDIO, codec,
>                                         delay_reporting, &endpoint_ind,
>                                         &cfm, sep);

avdtp.c shall not have dependencies on adapter.c, or any btd_ function
that is daemon specific.

> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 25520ceec..f2aa98b23 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -44,7 +44,6 @@
>  #define AVDTP_PSM 25
>
>  #define MAX_SEID 0x3E
> -static uint64_t seids;
>
>  #ifndef MAX
>  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> @@ -325,6 +324,7 @@ struct avdtp_local_sep {
>         GSList *caps;
>         struct avdtp_sep_ind *ind;
>         struct avdtp_sep_cfm *cfm;
> +       struct btd_adapter *adapter;

We should probably use the list (server->seps) instead to avoid
depending on the btd_adapter here.

>         void *user_data;
>  };
>
> @@ -414,6 +414,8 @@ struct avdtp {
>
>  static GSList *state_callbacks = NULL;
>
> +static GHashTable *adapter_seids = NULL;

I rather not use glib structures in new code.

>  static int send_request(struct avdtp *session, gboolean priority,
>                         struct avdtp_stream *stream, uint8_t signal_id,
>                         void *buffer, size_t size);
> @@ -3768,7 +3770,41 @@ int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
>                                                         &req, sizeof(req));
>  }
>
> -struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> +static uint8_t get_adapter_seid(struct btd_adapter *adapter)
> +{
> +       uint64_t *seids;
> +
> +       if (adapter_seids == NULL)
> +               adapter_seids = g_hash_table_new_full(g_direct_hash,
> +                                               g_direct_equal, NULL, g_free);
> +
> +       seids = g_hash_table_lookup(adapter_seids, adapter);
> +
> +       if (seids == NULL) {
> +               seids = g_new0(uint64_t, 1);
> +               g_hash_table_insert(adapter_seids, adapter, seids);
> +       }
> +
> +       return util_get_uid(seids, MAX_SEID);
> +}
> +
> +static void clear_adapter_seid(struct btd_adapter *adapter, uint8_t seid)
> +{
> +       uint64_t *seids = adapter_seids ?
> +                       g_hash_table_lookup(adapter_seids, adapter) : NULL;
> +
> +       if (seids == NULL)
> +               return;
> +
> +       util_clear_uid(seids, seid);
> +
> +       if (*seids == 0)
> +               g_hash_table_remove(adapter_seids, adapter);
> +}
> +
> +struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
> +                                               struct queue *lseps,
> +                                               uint8_t type,
>                                                 uint8_t media_type,
>                                                 uint8_t codec_type,
>                                                 gboolean delay_reporting,
> @@ -3777,7 +3813,7 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
>                                                 void *user_data)
>  {
>         struct avdtp_local_sep *sep;
> -       uint8_t seid = util_get_uid(&seids, MAX_SEID);
> +       uint8_t seid = get_adapter_seid(adapter);

Perhaps the uid pool should be passed instead of self generated by
avdtp.c, that way each server instance can contain its own seid pool
which can be passed to avdtp_register_sep, or better yet it can pass
the seid directly so the avdtp.c code is no longer responsible for
managing it and that is transfer to the caller which is already
managing the list anyway.

>
>         if (!seid)
>                 return NULL;
> @@ -3791,11 +3827,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
>         sep->codec = codec_type;
>         sep->ind = ind;
>         sep->cfm = cfm;
> +       sep->adapter = adapter;
>         sep->user_data = user_data;
>         sep->delay_reporting = delay_reporting;
>
> -       DBG("SEP %p registered: type:%d codec:%d seid:%d", sep,
> -                       sep->info.type, sep->codec, sep->info.seid);
> +       DBG("SEP %p registered: type:%d codec:%d adapter:%p seid:%d", sep,
> +                       sep->info.type, sep->codec, sep->adapter,
> +                       sep->info.seid);
>
>         if (!queue_push_tail(lseps, sep)) {
>                 g_free(sep);
> @@ -3813,10 +3851,11 @@ int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep)
>         if (sep->stream)
>                 release_stream(sep->stream, sep->stream->session);
>
> -       DBG("SEP %p unregistered: type:%d codec:%d seid:%d", sep,
> -                       sep->info.type, sep->codec, sep->info.seid);
> +       DBG("SEP %p unregistered: type:%d codec:%d adapter:%p seid:%d", sep,
> +                       sep->info.type, sep->codec, sep->adapter,
> +                       sep->info.seid);
>
> -       util_clear_uid(&seids, sep->info.seid);
> +       clear_adapter_seid(sep->adapter, sep->info.seid);
>         queue_remove(lseps, sep);
>         g_free(sep);
>
> diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> index b02534cd5..70807cff9 100644
> --- a/profiles/audio/avdtp.h
> +++ b/profiles/audio/avdtp.h
> @@ -278,7 +278,9 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream);
>  int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
>                                                         uint16_t delay);
>
> -struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> +struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
> +                                               struct queue *lseps,
> +                                               uint8_t type,
>                                                 uint8_t media_type,
>                                                 uint8_t codec_type,
>                                                 gboolean delay_reporting,
> --
> 2.31.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[Luiz Augusto von Dentz]

Hi Pauli,

On Sun, Aug 29, 2021 at 8:52 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> The util_get/clear_uid functions use int type for bitmap, and are used
> e.g. for SEID allocation. However, valid SEIDs are in range 1 to 0x3E
> (AVDTP spec v1.3, 8.20.1), and 8*sizeof(int) is often smaller than 0x3E.
>
> The function is also used in src/advertising.c, but an explicit maximum
> value is always provided, so growing the bitmap size is safe there.
>
> Use 64-bit bitmap instead, to be able to cover the valid range.
> ---
>  android/avdtp.c        |  2 +-
>  profiles/audio/avdtp.c |  2 +-
>  src/advertising.c      |  2 +-
>  src/shared/util.c      | 27 +++++++++++++++------------
>  src/shared/util.h      |  4 ++--
>  unit/test-avdtp.c      |  2 +-
>  6 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/android/avdtp.c b/android/avdtp.c
> index 8c2930ec1..a261a8e5f 100644
> --- a/android/avdtp.c
> +++ b/android/avdtp.c
> @@ -34,7 +34,7 @@
>  #include "../profiles/audio/a2dp-codecs.h"
>
>  #define MAX_SEID 0x3E
> -static unsigned int seids;
> +static uint64_t seids;
>
>  #ifndef MAX
>  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 946231b71..25520ceec 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -44,7 +44,7 @@
>  #define AVDTP_PSM 25
>
>  #define MAX_SEID 0x3E
> -static unsigned int seids;
> +static uint64_t seids;
>
>  #ifndef MAX
>  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> diff --git a/src/advertising.c b/src/advertising.c
> index bd79454d5..41b818650 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -48,7 +48,7 @@ struct btd_adv_manager {
>         uint8_t max_scan_rsp_len;
>         uint8_t max_ads;
>         uint32_t supported_flags;
> -       unsigned int instance_bitmap;
> +       uint64_t instance_bitmap;
>         bool extended_add_cmds;
>         int8_t min_tx_power;
>         int8_t max_tx_power;
> diff --git a/src/shared/util.c b/src/shared/util.c
> index 244756456..723dedd75 100644
> --- a/src/shared/util.c
> +++ b/src/shared/util.c
> @@ -124,30 +124,33 @@ unsigned char util_get_dt(const char *parent, const char *name)
>
>  /* Helpers for bitfield operations */
>
> -/* Find unique id in range from 1 to max but no bigger then
> - * sizeof(int) * 8. ffs() is used since it is POSIX standard
> - */
> -uint8_t util_get_uid(unsigned int *bitmap, uint8_t max)
> +/* Find unique id in range from 1 to max but no bigger than 64. */
> +uint8_t util_get_uid(uint64_t *bitmap, uint8_t max)
>  {
>         uint8_t id;
>
> -       id = ffs(~*bitmap);

Can't we use ffsll instead of using a for loop testing every bit?
Afaik long long should be at least 64 bits.

> +       if (max > 64)
> +               max = 64;
>
> -       if (!id || id > max)
> -               return 0;
> +       for (id = 1; id <= max; ++id) {
> +               uint64_t mask = ((uint64_t)1) << (id - 1);
>
> -       *bitmap |= 1u << (id - 1);
> +               if (!(*bitmap & mask)) {
> +                       *bitmap |= mask;
> +                       return id;
> +               }
> +       }
>
> -       return id;
> +       return 0;
>  }
>
>  /* Clear id bit in bitmap */
> -void util_clear_uid(unsigned int *bitmap, uint8_t id)
> +void util_clear_uid(uint64_t *bitmap, uint8_t id)
>  {
> -       if (!id)
> +       if (id == 0 || id > 64)
>                 return;
>
> -       *bitmap &= ~(1u << (id - 1));
> +       *bitmap &= ~(((uint64_t)1) << (id - 1));
>  }
>
>  static const struct {
> diff --git a/src/shared/util.h b/src/shared/util.h
> index 9920b7f76..60908371d 100644
> --- a/src/shared/util.h
> +++ b/src/shared/util.h
> @@ -102,8 +102,8 @@ void util_hexdump(const char dir, const unsigned char *buf, size_t len,
>
>  unsigned char util_get_dt(const char *parent, const char *name);
>
> -uint8_t util_get_uid(unsigned int *bitmap, uint8_t max);
> -void util_clear_uid(unsigned int *bitmap, uint8_t id);
> +uint8_t util_get_uid(uint64_t *bitmap, uint8_t max);
> +void util_clear_uid(uint64_t *bitmap, uint8_t id);
>
>  const char *bt_uuid16_to_str(uint16_t uuid);
>  const char *bt_uuid32_to_str(uint32_t uuid);
> diff --git a/unit/test-avdtp.c b/unit/test-avdtp.c
> index f5340d6f3..4e8a68c6b 100644
> --- a/unit/test-avdtp.c
> +++ b/unit/test-avdtp.c
> @@ -550,7 +550,7 @@ static void test_server_seid(gconstpointer data)
>         struct avdtp_local_sep *sep;
>         unsigned int i;
>
> -       for (i = 0; i < sizeof(int) * 8; i++) {
> +       for (i = 0; i < MAX_SEID; i++) {
>                 sep = avdtp_register_sep(context->lseps, AVDTP_SEP_TYPE_SINK,
>                                                 AVDTP_MEDIA_TYPE_AUDIO,
>                                                 0x00, TRUE, &sep_ind, NULL,
> --
> 2.31.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[Pauli Virtanen]

Hi Luiz,

pe, 2021-09-03 kello 15:59 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Aug 29, 2021 at 8:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > The util_get/clear_uid functions use int type for bitmap, and are used
> > e.g. for SEID allocation. However, valid SEIDs are in range 1 to 0x3E
> > (AVDTP spec v1.3, 8.20.1), and 8*sizeof(int) is often smaller than 0x3E.
> > 
> > The function is also used in src/advertising.c, but an explicit maximum
> > value is always provided, so growing the bitmap size is safe there.
> > 
> > Use 64-bit bitmap instead, to be able to cover the valid range.
> > ---
> >  android/avdtp.c        |  2 +-
> >  profiles/audio/avdtp.c |  2 +-
> >  src/advertising.c      |  2 +-
> >  src/shared/util.c      | 27 +++++++++++++++------------
> >  src/shared/util.h      |  4 ++--
> >  unit/test-avdtp.c      |  2 +-
> >  6 files changed, 21 insertions(+), 18 deletions(-)
> > 
> > diff --git a/android/avdtp.c b/android/avdtp.c
> > index 8c2930ec1..a261a8e5f 100644
> > --- a/android/avdtp.c
> > +++ b/android/avdtp.c
> > @@ -34,7 +34,7 @@
> >  #include "../profiles/audio/a2dp-codecs.h"
> > 
> >  #define MAX_SEID 0x3E
> > -static unsigned int seids;
> > +static uint64_t seids;
> > 
> >  #ifndef MAX
> >  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index 946231b71..25520ceec 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -44,7 +44,7 @@
> >  #define AVDTP_PSM 25
> > 
> >  #define MAX_SEID 0x3E
> > -static unsigned int seids;
> > +static uint64_t seids;
> > 
> >  #ifndef MAX
> >  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> > diff --git a/src/advertising.c b/src/advertising.c
> > index bd79454d5..41b818650 100644
> > --- a/src/advertising.c
> > +++ b/src/advertising.c
> > @@ -48,7 +48,7 @@ struct btd_adv_manager {
> >         uint8_t max_scan_rsp_len;
> >         uint8_t max_ads;
> >         uint32_t supported_flags;
> > -       unsigned int instance_bitmap;
> > +       uint64_t instance_bitmap;
> >         bool extended_add_cmds;
> >         int8_t min_tx_power;
> >         int8_t max_tx_power;
> > diff --git a/src/shared/util.c b/src/shared/util.c
> > index 244756456..723dedd75 100644
> > --- a/src/shared/util.c
> > +++ b/src/shared/util.c
> > @@ -124,30 +124,33 @@ unsigned char util_get_dt(const char *parent, const char *name)
> > 
> >  /* Helpers for bitfield operations */
> > 
> > -/* Find unique id in range from 1 to max but no bigger then
> > - * sizeof(int) * 8. ffs() is used since it is POSIX standard
> > - */
> > -uint8_t util_get_uid(unsigned int *bitmap, uint8_t max)
> > +/* Find unique id in range from 1 to max but no bigger than 64. */
> > +uint8_t util_get_uid(uint64_t *bitmap, uint8_t max)
> >  {
> >         uint8_t id;
> > 
> > -       id = ffs(~*bitmap);
> 
> Can't we use ffsll instead of using a for loop testing every bit?
> Afaik long long should be at least 64 bits.

Ok, I now see GNU extensions are fine here. I'll use ffsll to make it
simpler.

> > +       if (max > 64)
> > +               max = 64;
> > 
> > -       if (!id || id > max)
> > -               return 0;
> > +       for (id = 1; id <= max; ++id) {
> > +               uint64_t mask = ((uint64_t)1) << (id - 1);
> > 
> > -       *bitmap |= 1u << (id - 1);
> > +               if (!(*bitmap & mask)) {
> > +                       *bitmap |= mask;
> > +                       return id;
> > +               }
> > +       }
> > 
> > -       return id;
> > +       return 0;
> >  }
> > 
> >  /* Clear id bit in bitmap */
> > -void util_clear_uid(unsigned int *bitmap, uint8_t id)
> > +void util_clear_uid(uint64_t *bitmap, uint8_t id)
> >  {
> > -       if (!id)
> > +       if (id == 0 || id > 64)
> >                 return;
> > 
> > -       *bitmap &= ~(1u << (id - 1));
> > +       *bitmap &= ~(((uint64_t)1) << (id - 1));
> >  }
> > 
> >  static const struct {
> > diff --git a/src/shared/util.h b/src/shared/util.h
> > index 9920b7f76..60908371d 100644
> > --- a/src/shared/util.h
> > +++ b/src/shared/util.h
> > @@ -102,8 +102,8 @@ void util_hexdump(const char dir, const
> > unsigned char *buf, size_t len,
> > 
> >  unsigned char util_get_dt(const char *parent, const char *name);
> > 
> > -uint8_t util_get_uid(unsigned int *bitmap, uint8_t max);
> > -void util_clear_uid(unsigned int *bitmap, uint8_t id);
> > +uint8_t util_get_uid(uint64_t *bitmap, uint8_t max);
> > +void util_clear_uid(uint64_t *bitmap, uint8_t id);
> > 
> >  const char *bt_uuid16_to_str(uint16_t uuid);
> >  const char *bt_uuid32_to_str(uint32_t uuid);
> > diff --git a/unit/test-avdtp.c b/unit/test-avdtp.c
> > index f5340d6f3..4e8a68c6b 100644
> > --- a/unit/test-avdtp.c
> > +++ b/unit/test-avdtp.c
> > @@ -550,7 +550,7 @@ static void test_server_seid(gconstpointer
> > data)
> >         struct avdtp_local_sep *sep;
> >         unsigned int i;
> > 
> > -       for (i = 0; i < sizeof(int) * 8; i++) {
> > +       for (i = 0; i < MAX_SEID; i++) {
> >                 sep = avdtp_register_sep(context->lseps,
> > AVDTP_SEP_TYPE_SINK,
> >                                                 AVDTP_MEDIA_TYPE_AU
> > DIO,
> >                                                 0x00, TRUE,
> > &sep_ind, NULL,
> > --
> > 2.31.1
> > 
> 
> 

Re: [PATCH BlueZ 2/2] avdtp: use separate local SEID pool for each adapter

[Pauli Virtanen]

Hi Luiz,

pe, 2021-09-03 kello 15:49 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Aug 29, 2021 at 8:52 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Local SEIDs are currently allocated from a pool that is common for all
> > adapters. However, AVDTP spec v1.3, sec 4.10 states "To prevent
> > conflicts, the scope of the SEID shall be both device-local and
> > connection-local. The application is responsible for assigning a SEID,
> > which is not in use on the connection to the same peer device." In
> > practice, registering the same media application for multiple adapters
> > can result to running out of SEIDs, even though the spec does not
> > require SEIDs to be unique across adapters.
> > 
> > Use a separate SEID pool for each btd_adapter to fix this.
> > ---
> >  profiles/audio/a2dp.c  |  2 +-
> >  profiles/audio/avdtp.c | 55 ++++++++++++++++++++++++++++++++++++------
> >  profiles/audio/avdtp.h |  4 ++-
> >  3 files changed, 51 insertions(+), 10 deletions(-)
> > 
> > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
> > index 02caa83e1..1e8a66b8a 100644
> > --- a/profiles/audio/a2dp.c
> > +++ b/profiles/audio/a2dp.c
> > @@ -2615,7 +2615,7 @@ struct a2dp_sep *a2dp_add_sep(struct btd_adapter *adapter, uint8_t type,
> > 
> >         sep = g_new0(struct a2dp_sep, 1);
> > 
> > -       sep->lsep = avdtp_register_sep(server->seps, type,
> > +       sep->lsep = avdtp_register_sep(adapter, server->seps, type,
> >                                         AVDTP_MEDIA_TYPE_AUDIO, codec,
> >                                         delay_reporting, &endpoint_ind,
> >                                         &cfm, sep);
> 
> avdtp.c shall not have dependencies on adapter.c, or any btd_ function
> that is daemon specific.

Ack.

> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index 25520ceec..f2aa98b23 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -44,7 +44,6 @@
> >  #define AVDTP_PSM 25
> > 
> >  #define MAX_SEID 0x3E
> > -static uint64_t seids;
> > 
> >  #ifndef MAX
> >  # define MAX(x, y) ((x) > (y) ? (x) : (y))
> > @@ -325,6 +324,7 @@ struct avdtp_local_sep {
> >         GSList *caps;
> >         struct avdtp_sep_ind *ind;
> >         struct avdtp_sep_cfm *cfm;
> > +       struct btd_adapter *adapter;
> 
> We should probably use the list (server->seps) instead to avoid
> depending on the btd_adapter here.

This would mean that a2dp_server owns the SEID pool.

a2dp_server is the only local SEP user, and there's 1-to-1
correspondence with adapters, so that should work currently. But I
don't know (so far...) the big picture / plans for BlueZ design, so not
yet clear to me who should own the pool.

If a2dp_server owns it, as you write below, it's better then to just
have it own the bitmap.

> 
> >         void *user_data;
> >  };
> > 
> > @@ -414,6 +414,8 @@ struct avdtp {
> > 
> >  static GSList *state_callbacks = NULL;
> > 
> > +static GHashTable *adapter_seids = NULL;
> 
> I rather not use glib structures in new code.
> 
> >  static int send_request(struct avdtp *session, gboolean priority,
> >                         struct avdtp_stream *stream, uint8_t signal_id,
> >                         void *buffer, size_t size);
> > @@ -3768,7 +3770,41 @@ int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
> >                                                         &req, sizeof(req));
> >  }
> > 
> > -struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> > +static uint8_t get_adapter_seid(struct btd_adapter *adapter)
> > +{
> > +       uint64_t *seids;
> > +
> > +       if (adapter_seids == NULL)
> > +               adapter_seids = g_hash_table_new_full(g_direct_hash,
> > +                                               g_direct_equal, NULL, g_free);
> > +
> > +       seids = g_hash_table_lookup(adapter_seids, adapter);
> > +
> > +       if (seids == NULL) {
> > +               seids = g_new0(uint64_t, 1);
> > +               g_hash_table_insert(adapter_seids, adapter, seids);
> > +       }
> > +
> > +       return util_get_uid(seids, MAX_SEID);
> > +}
> > +
> > +static void clear_adapter_seid(struct btd_adapter *adapter, uint8_t seid)
> > +{
> > +       uint64_t *seids = adapter_seids ?
> > +                       g_hash_table_lookup(adapter_seids, adapter) : NULL;
> > +
> > +       if (seids == NULL)
> > +               return;
> > +
> > +       util_clear_uid(seids, seid);
> > +
> > +       if (*seids == 0)
> > +               g_hash_table_remove(adapter_seids, adapter);
> > +}
> > +
> > +struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
> > +                                               struct queue *lseps,
> > +                                               uint8_t type,
> >                                                 uint8_t media_type,
> >                                                 uint8_t codec_type,
> >                                                 gboolean delay_reporting,
> > @@ -3777,7 +3813,7 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> >                                                 void *user_data)
> >  {
> >         struct avdtp_local_sep *sep;
> > -       uint8_t seid = util_get_uid(&seids, MAX_SEID);
> > +       uint8_t seid = get_adapter_seid(adapter);
> 
> Perhaps the uid pool should be passed instead of self generated by
> avdtp.c, that way each server instance can contain its own seid pool
> which can be passed to avdtp_register_sep, or better yet it can pass
> the seid directly so the avdtp.c code is no longer responsible for
> managing it and that is transfer to the caller which is already
> managing the list anyway.

I'll change this to a2dp_server owning the bitmap.

For knowledge of MAX_SEID and error handling, it may be cleaner if the
pool is passed in.

> > 
> >         if (!seid)
> >                 return NULL;
> > @@ -3791,11 +3827,13 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> >         sep->codec = codec_type;
> >         sep->ind = ind;
> >         sep->cfm = cfm;
> > +       sep->adapter = adapter;
> >         sep->user_data = user_data;
> >         sep->delay_reporting = delay_reporting;
> > 
> > -       DBG("SEP %p registered: type:%d codec:%d seid:%d", sep,
> > -                       sep->info.type, sep->codec, sep->info.seid);
> > +       DBG("SEP %p registered: type:%d codec:%d adapter:%p seid:%d", sep,
> > +                       sep->info.type, sep->codec, sep->adapter,
> > +                       sep->info.seid);
> > 
> >         if (!queue_push_tail(lseps, sep)) {
> >                 g_free(sep);
> > @@ -3813,10 +3851,11 @@ int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep)
> >         if (sep->stream)
> >                 release_stream(sep->stream, sep->stream->session);
> > 
> > -       DBG("SEP %p unregistered: type:%d codec:%d seid:%d", sep,
> > -                       sep->info.type, sep->codec, sep->info.seid);
> > +       DBG("SEP %p unregistered: type:%d codec:%d adapter:%p seid:%d", sep,
> > +                       sep->info.type, sep->codec, sep->adapter,
> > +                       sep->info.seid);
> > 
> > -       util_clear_uid(&seids, sep->info.seid);
> > +       clear_adapter_seid(sep->adapter, sep->info.seid);
> >         queue_remove(lseps, sep);
> >         g_free(sep);
> > 
> > diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
> > index b02534cd5..70807cff9 100644
> > --- a/profiles/audio/avdtp.h
> > +++ b/profiles/audio/avdtp.h
> > @@ -278,7 +278,9 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream);
> >  int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
> >                                                         uint16_t delay);
> > 
> > -struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
> > +struct avdtp_local_sep *avdtp_register_sep(struct btd_adapter *adapter,
> > +                                               struct queue *lseps,
> > +                                               uint8_t type,
> >                                                 uint8_t media_type,
> >                                                 uint8_t codec_type,
> >                                                 gboolean delay_reporting,
> > --
> > 2.31.1
> > 
> 
> 

-- 
Pauli Virtanen

[PATCH BlueZ v2 1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[Pauli Virtanen]

The util_get/clear_uid functions use int type for bitmap, and are used
e.g. for SEID allocation. However, valid SEIDs are in range 1 to 0x3E
(AVDTP spec v1.3, 8.20.1), and 8*sizeof(int) is often smaller than 0x3E.

The function is also used in src/advertising.c, but an explicit maximum
value is always provided, so growing the bitmap size is safe there.

Use 64-bit bitmap instead, to be able to cover the valid range.
---
Changes in v2:
* Use ffsll

 android/avdtp.c        |  2 +-
 profiles/audio/avdtp.c |  2 +-
 src/advertising.c      |  2 +-
 src/shared/util.c      | 16 +++++++---------
 src/shared/util.h      |  4 ++--
 unit/test-avdtp.c      |  2 +-
 6 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index 8c2930ec1..a261a8e5f 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -34,7 +34,7 @@
 #include "../profiles/audio/a2dp-codecs.h"
 
 #define MAX_SEID 0x3E
-static unsigned int seids;
+static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 946231b71..25520ceec 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -44,7 +44,7 @@
 #define AVDTP_PSM 25
 
 #define MAX_SEID 0x3E
-static unsigned int seids;
+static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
diff --git a/src/advertising.c b/src/advertising.c
index bd79454d5..41b818650 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -48,7 +48,7 @@ struct btd_adv_manager {
 	uint8_t max_scan_rsp_len;
 	uint8_t max_ads;
 	uint32_t supported_flags;
-	unsigned int instance_bitmap;
+	uint64_t instance_bitmap;
 	bool extended_add_cmds;
 	int8_t min_tx_power;
 	int8_t max_tx_power;
diff --git a/src/shared/util.c b/src/shared/util.c
index 244756456..2887a3efa 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -124,30 +124,28 @@ unsigned char util_get_dt(const char *parent, const char *name)
 
 /* Helpers for bitfield operations */
 
-/* Find unique id in range from 1 to max but no bigger then
- * sizeof(int) * 8. ffs() is used since it is POSIX standard
- */
-uint8_t util_get_uid(unsigned int *bitmap, uint8_t max)
+/* Find unique id in range from 1 to max but no bigger than 64. */
+uint8_t util_get_uid(uint64_t *bitmap, uint8_t max)
 {
 	uint8_t id;
 
-	id = ffs(~*bitmap);
+	id = ffsll(~*bitmap);
 
 	if (!id || id > max)
 		return 0;
 
-	*bitmap |= 1u << (id - 1);
+	*bitmap |= ((uint64_t)1) << (id - 1);
 
 	return id;
 }
 
 /* Clear id bit in bitmap */
-void util_clear_uid(unsigned int *bitmap, uint8_t id)
+void util_clear_uid(uint64_t *bitmap, uint8_t id)
 {
-	if (!id)
+	if (!id || id > 64)
 		return;
 
-	*bitmap &= ~(1u << (id - 1));
+	*bitmap &= ~(((uint64_t)1) << (id - 1));
 }
 
 static const struct {
diff --git a/src/shared/util.h b/src/shared/util.h
index 9920b7f76..60908371d 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -102,8 +102,8 @@ void util_hexdump(const char dir, const unsigned char *buf, size_t len,
 
 unsigned char util_get_dt(const char *parent, const char *name);
 
-uint8_t util_get_uid(unsigned int *bitmap, uint8_t max);
-void util_clear_uid(unsigned int *bitmap, uint8_t id);
+uint8_t util_get_uid(uint64_t *bitmap, uint8_t max);
+void util_clear_uid(uint64_t *bitmap, uint8_t id);
 
 const char *bt_uuid16_to_str(uint16_t uuid);
 const char *bt_uuid32_to_str(uint32_t uuid);
diff --git a/unit/test-avdtp.c b/unit/test-avdtp.c
index f5340d6f3..4e8a68c6b 100644
--- a/unit/test-avdtp.c
+++ b/unit/test-avdtp.c
@@ -550,7 +550,7 @@ static void test_server_seid(gconstpointer data)
 	struct avdtp_local_sep *sep;
 	unsigned int i;
 
-	for (i = 0; i < sizeof(int) * 8; i++) {
+	for (i = 0; i < MAX_SEID; i++) {
 		sep = avdtp_register_sep(context->lseps, AVDTP_SEP_TYPE_SINK,
 						AVDTP_MEDIA_TYPE_AUDIO,
 						0x00, TRUE, &sep_ind, NULL,
-- 
2.31.1

[PATCH BlueZ v2 2/2] avdtp: use separate local SEID pool for each adapter

[Pauli Virtanen]

Local SEIDs are currently allocated from a pool that is common for all
adapters. However, AVDTP spec v1.3, sec 4.10 states "To prevent
conflicts, the scope of the SEID shall be both device-local and
connection-local. The application is responsible for assigning a SEID,
which is not in use on the connection to the same peer device." In
practice, registering the same media application for multiple adapters
can result to running out of SEIDs, even though the spec does not
require SEIDs to be unique across adapters.

To fix this, have a2dp_server own the SEID pool and pass it to avdtp
functions. Currently, a2dp_server is the only one that registers local
SEPs, and its correspondence to adapters is unique, so it can own the
pool.
---
Changes in v2:
* Store SEID pool in a2dp_server, passed to avdtp functions.
  Otherwise SEID pool handling is done in avdtp.c, keeping it
  similar to what is done with lseps, and not requiring
  caller to know MAX_SEID.

 profiles/audio/a2dp.c  |  5 +++--
 profiles/audio/avdtp.c | 23 ++++++++++++++---------
 profiles/audio/avdtp.h |  7 +++++--
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index 02caa83e1..9d3aaa136 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -118,6 +118,7 @@ struct a2dp_server {
 	uint32_t sink_record_id;
 	gboolean sink_enabled;
 	gboolean source_enabled;
+	uint64_t seid_pool;
 	GIOChannel *io;
 	struct queue *seps;
 	struct queue *channels;
@@ -2553,7 +2554,7 @@ static void a2dp_unregister_sep(struct a2dp_sep *sep)
 		sep->endpoint = NULL;
 	}
 
-	avdtp_unregister_sep(server->seps, sep->lsep);
+	avdtp_unregister_sep(server->seps, &server->seid_pool, sep->lsep);
 
 	g_free(sep);
 
@@ -2615,7 +2616,7 @@ struct a2dp_sep *a2dp_add_sep(struct btd_adapter *adapter, uint8_t type,
 
 	sep = g_new0(struct a2dp_sep, 1);
 
-	sep->lsep = avdtp_register_sep(server->seps, type,
+	sep->lsep = avdtp_register_sep(server->seps, &server->seid_pool, type,
 					AVDTP_MEDIA_TYPE_AUDIO, codec,
 					delay_reporting, &endpoint_ind,
 					&cfm, sep);
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 25520ceec..d3dfbf96d 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -44,7 +44,6 @@
 #define AVDTP_PSM 25
 
 #define MAX_SEID 0x3E
-static uint64_t seids;
 
 #ifndef MAX
 # define MAX(x, y) ((x) > (y) ? (x) : (y))
@@ -3768,7 +3767,9 @@ int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
 							&req, sizeof(req));
 }
 
-struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
+struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps,
+						uint64_t *seid_pool,
+						uint8_t type,
 						uint8_t media_type,
 						uint8_t codec_type,
 						gboolean delay_reporting,
@@ -3777,7 +3778,7 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
 						void *user_data)
 {
 	struct avdtp_local_sep *sep;
-	uint8_t seid = util_get_uid(&seids, MAX_SEID);
+	uint8_t seid = util_get_uid(seid_pool, MAX_SEID);
 
 	if (!seid)
 		return NULL;
@@ -3794,18 +3795,21 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
 	sep->user_data = user_data;
 	sep->delay_reporting = delay_reporting;
 
-	DBG("SEP %p registered: type:%d codec:%d seid:%d", sep,
-			sep->info.type, sep->codec, sep->info.seid);
+	DBG("SEP %p registered: type:%d codec:%d seid_pool:%p seid:%d", sep,
+			sep->info.type, sep->codec, seid_pool,
+			sep->info.seid);
 
 	if (!queue_push_tail(lseps, sep)) {
 		g_free(sep);
+		util_clear_uid(seid_pool, seid);
 		return NULL;
 	}
 
 	return sep;
 }
 
-int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep)
+int avdtp_unregister_sep(struct queue *lseps, uint64_t *seid_pool,
+						struct avdtp_local_sep *sep)
 {
 	if (!sep)
 		return -EINVAL;
@@ -3813,10 +3817,11 @@ int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep)
 	if (sep->stream)
 		release_stream(sep->stream, sep->stream->session);
 
-	DBG("SEP %p unregistered: type:%d codec:%d seid:%d", sep,
-			sep->info.type, sep->codec, sep->info.seid);
+	DBG("SEP %p unregistered: type:%d codec:%d seid_pool:%p seid:%d", sep,
+			sep->info.type, sep->codec, seid_pool,
+			sep->info.seid);
 
-	util_clear_uid(&seids, sep->info.seid);
+	util_clear_uid(seid_pool, sep->info.seid);
 	queue_remove(lseps, sep);
 	g_free(sep);
 
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index b02534cd5..102a2603e 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -278,7 +278,9 @@ int avdtp_abort(struct avdtp *session, struct avdtp_stream *stream);
 int avdtp_delay_report(struct avdtp *session, struct avdtp_stream *stream,
 							uint16_t delay);
 
-struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
+struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps,
+						uint64_t *seid_pool,
+						uint8_t type,
 						uint8_t media_type,
 						uint8_t codec_type,
 						gboolean delay_reporting,
@@ -290,7 +292,8 @@ struct avdtp_local_sep *avdtp_register_sep(struct queue *lseps, uint8_t type,
 struct avdtp_remote_sep *avdtp_find_remote_sep(struct avdtp *session,
 						struct avdtp_local_sep *lsep);
 
-int avdtp_unregister_sep(struct queue *lseps, struct avdtp_local_sep *sep);
+int avdtp_unregister_sep(struct queue *lseps, uint64_t *seid_pool,
+						struct avdtp_local_sep *sep);
 
 avdtp_state_t avdtp_sep_get_state(struct avdtp_local_sep *sep);
 uint8_t avdtp_sep_get_seid(struct avdtp_local_sep *sep);
-- 
2.31.1

RE: [BlueZ,v2,1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

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=542233

---Test result---

Test Summary:
CheckPatch                    PASS      0.30 seconds
GitLint                       PASS      0.11 seconds
Prep - Setup ELL              PASS      44.86 seconds
Build - Prep                  PASS      0.10 seconds
Build - Configure             PASS      7.75 seconds
Build - Make                  PASS      192.81 seconds
Make Check                    PASS      9.37 seconds
Make Distcheck                PASS      229.33 seconds
Build w/ext ELL - Configure   PASS      8.02 seconds
Build w/ext ELL - Make        PASS      181.60 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

Re: [BlueZ,v2,1/2] shared/util: use 64-bit bitmap in util_get/clear_uid

[Luiz Augusto von Dentz]

Hi Pauli,

On Sun, Sep 5, 2021 at 10:01 AM <bluez.test.bot@gmail.com> wrote:
>
> 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=542233
>
> ---Test result---
>
> Test Summary:
> CheckPatch                    PASS      0.30 seconds
> GitLint                       PASS      0.11 seconds
> Prep - Setup ELL              PASS      44.86 seconds
> Build - Prep                  PASS      0.10 seconds
> Build - Configure             PASS      7.75 seconds
> Build - Make                  PASS      192.81 seconds
> Make Check                    PASS      9.37 seconds
> Make Distcheck                PASS      229.33 seconds
> Build w/ext ELL - Configure   PASS      8.02 seconds
> Build w/ext ELL - Make        PASS      181.60 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

Applied, thanks.

-- 
Luiz Augusto von Dentz