| Message ID | 20210829155012.164880-3-pav@iki.fi (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | AVDTP SEID pool seems too small | expand |
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 >
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 > > > >
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,