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