Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3681-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Vlad Pruteanu <vlad.pruteanu@nxp.com>
To: linux-bluetooth@vger.kernel.org
Cc: claudia.rosu@nxp.com,
	mihai-octavian.urzica@nxp.com,
	silviu.barbulescu@nxp.com,
	iulia.tanasescu@nxp.com,
	andrei.istodorescu@nxp.com,
	luiz.dentz@gmail.com,
	Vlad Pruteanu <vlad.pruteanu@nxp.com>
Subject: [PATCH BlueZ 1/1] bap: Replace global bcast_pa_requests with a queue for each adapter
Date: Wed, 17 Apr 2024 17:25:37 +0300
Message-Id: <20240417142537.6523-2-vlad.pruteanu@nxp.com>
In-Reply-To: <20240417142537.6523-1-vlad.pruteanu@nxp.com>
References: <20240417142537.6523-1-vlad.pruteanu@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?fV2+9PePHQ8e9hdGxQ+7g9lwkdaDjQvwXUG1BsW/uEq2OUH4+N3o67f7vNMN?=
 =?us-ascii?Q?W0+NPDYDif2ADkxb+DgGQGlEZdAo6jzIFaVeASSSFk80TgSik6c/OYusCvPg?=
 =?us-ascii?Q?TIstcfh0Z/UuEf80BdSXz5J2hi8N1s4LtGQWlcOWXGi1eychyd2N0L+mMYX3?=
 =?us-ascii?Q?Ra/kggHevX8dGSruoqCKkC1pVtMzsPR74/QTQdfIJRJl5qznBhH3PFe5ePFC?=
 =?us-ascii?Q?jZz1FMrn14ksAAAj+jUcet7ZovRSrbk2HJJEIoRykTPMtDvL6hDAX3QUavQB?=
 =?us-ascii?Q?wqyM/a8J9gPh+6RAUwbirme9L10QsbRcHqXiJWgxedrW6oPoYkIX6UAI7wU5?=
 =?us-ascii?Q?uqUr93mNGCQAQBEHyI/uoRRMZF+GmB7v0mT9MUT5eaDTIHmnZyptdGegseOy?=
 =?us-ascii?Q?xW/Zxx92xM8FfzIcnzM3PkwT/u1y1wscUMG5q7PGxzwpBqUi8IGp/VMtCQ/8?=
 =?us-ascii?Q?UO/j81ZW7Bi0GMe6AQPY54tioZMisdOMvPvwWNKOtaK2Ph7alkHrzZo1tKko?=
 =?us-ascii?Q?ZH5ZwwSwiNRA1cvUo4MxbzVu29t5P7NJgXFe4fy0vKrXp/KKGRiSL6T+Cgia?=
 =?us-ascii?Q?SIPjE4uozv5PX2notopvChZVmPnJM7gw4V/r0Mwl1m+7x4yjnh0+V2rGcp4L?=
 =?us-ascii?Q?O1Ip4pF3u/HQ8ZNCX3vnLGl/C5+j83usb1efEJ5RdBHUwle8dsJVD22FS7dJ?=
 =?us-ascii?Q?GScoaDF7NLaRfZn6ZtqgEL4BUEOTq4dHiuQeVk598lT60OzuQM6wr3uCdIVg?=
 =?us-ascii?Q?Uj+8QewiCbKexX3aSkSo1ltG9IdK9RqP4yWIXsvitKtcBcCH1DiFKHq0K3x1?=
 =?us-ascii?Q?+eDVTGPixZdINw2bkGlXUBOurXqMgxA8+IfYD8fIfFuAisIxGN6XIU07sRZU?=
 =?us-ascii?Q?djhnE5CU9IXOW77hTbDuVm/wqjST5JnKk7Y2iDNvoEpISndZdM4LkCqRWl1l?=
 =?us-ascii?Q?4cwlgI3iN9z2XwNyHrx40+/w3IcFapW7sakXHIKx6KxCHzSvCnLn67VxqLFH?=
 =?us-ascii?Q?KuWlzSjKkafvJO6k4jF4eWI68gM3XgFbPwm/vhnQuf1y3KMhplJ5c9kNPaSR?=
 =?us-ascii?Q?3pEQMnnDKSvwlTeQ/wjNPcZjT3J8CNFBWtNXmF+n7yloP5gXqjwQw6X4R/lv?=
 =?us-ascii?Q?Jc2Cv2Fg6uc4BLFYeBXlLHkYLbwhfqBtqYxyx/32b2G9oBiAUEBB9vUTuuX7?=
 =?us-ascii?Q?XihFO3S6/5Lk4HFxtj0Q6g5YmcD9zYTeMC58yWcRiTZxZU62mMtGT+N3CqsH?=
 =?us-ascii?Q?3JTpr8/N4LrHd+wS0pPhBi1+TvlRpPpItuidGGApJQ/Hyh05NZmFtpiGgiP0?=
 =?us-ascii?Q?QQj+gFm6qOY3qpradVVJ2N2aibnm31A16GQtrhPh7aSTmhM1hk98UWYQtgIb?=
 =?us-ascii?Q?h/mldJL2YxzRKuOuA6flL7v2qU2dVUj+2iy/FmT8Iku6P4RmNS5SMtBMjlQp?=
 =?us-ascii?Q?f+n0I4uFowRmSwSsckGvkfQWq/NOyox7YXzdWe1RXoAs4yJ2GqnGRUrpgq+b?=
 =?us-ascii?Q?rgr2HxxfrPxvXK6LVYNTXW6oq3xCRh5qf4yABO5WFHMz5IC20ZgH48/PKfxE?=
 =?us-ascii?Q?wBaQPLhT7MASC7JLrPmGEEte2FDkm3H1LGi95pDc?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 12d6641f-8036-460f-3150-08dc5eea4e71
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5389.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 14:26:01.3075
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RjFZ37KFdT/cmWnACiL9G2IrAMhG54UPL+EFx1AfdnqXrX18MKLuGZHgvaMur/NvXqcZNqUWYZoSiDppdMmDOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB7504

This patch replaces the current implementation that uses a global
bcast_pa_requests queue and instead creates a queue and a timer for
each adapter.

The queue for a specific adapter, the timer's id and the adapter's id
form a new struct called pa_timer_data. For each adapter a new instance
of this struct is created and inserted into the global queue, pa_timers.

Operations on the old bcast_pa_requests queue have been modified such
that:
1) based on the adapter_id, the correct entry in the pa_timers queue is
retrieved
2) the operation can be called on the bcast_pa_requests queue from the
resulting pa_timer_data instance

The timers are created and stopped on a per adapter basis. A timer is
stopped if the adapter's pa_req queue is empty. A pa_timer_id equal
to 0 means that the timer is stopped.

The bap_bcast_remove function has been updated to remove from the
pa_req queue entries of devices that were freed. pa_req that are already
in progress are treated by the bap_data_free function.

This patch also fixes a crash that occurs when a device is freed before
the pa_idle_timer handles it's entry in the pa_req queue. The following
log was obtained while running an Unicast setup:

==105052==ERROR: AddressSanitizer: heap-use-after-free on address
0x60400001c418 at pc 0x55775caf1846 bp 0x7ffc83d9fb90 sp 0x7ffc83d9fb80
READ of size 8 at 0x60400001c418 thread T0
0 0x55775caf1845 in btd_service_get_device src/service.c:325
1 0x55775ca03da2 in short_lived_pa_sync profiles/audio/bap.c:2693
2 0x55775ca03da2 in pa_idle_timer profiles/audio/bap.c:1996
---
 profiles/audio/bap.c | 115 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 97 insertions(+), 18 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index db0af7e7c..df0c35a6f 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -126,9 +126,14 @@ struct bap_bcast_pa_req {
 	} data;
 };
 
+struct pa_timer_data {
+	struct btd_adapter *adapter;
+	unsigned int pa_timer_id;
+	struct queue *bcast_pa_requests;
+};
+
 static struct queue *sessions;
-static struct queue *bcast_pa_requests;
-static unsigned int pa_timer_id;
+static struct queue *pa_timers;
 
 /* Structure holding the parameters for Periodic Advertisement create sync.
  * The full QOS is populated at the time the user selects and endpoint and
@@ -965,15 +970,25 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 	return NULL;
 }
 
+static bool pa_timer_match_adapter(const void *data, const void *match_data)
+{
+	struct pa_timer_data *pa_timer = (struct pa_timer_data *)data;
+
+	return pa_timer->adapter == match_data;
+}
+
 static void iso_bcast_confirm_cb(GIOChannel *io, GError *err, void *user_data)
 {
 	struct bap_bcast_pa_req *req = user_data;
 	struct bap_setup *setup = req->data.setup;
+	struct pa_timer_data *pa_timer = queue_find(pa_timers,
+			pa_timer_match_adapter, setup->ep->data->adapter);
 	int fd;
 
 	DBG("BIG Sync completed");
 
-	queue_remove(bcast_pa_requests, req);
+	queue_remove(pa_timer->bcast_pa_requests, req);
+	free(req);
 
 	/* This device is no longer needed */
 	btd_service_connecting_complete(setup->ep->data->service, 0);
@@ -1111,6 +1126,8 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
 	struct bap_data *data = btd_service_get_user_data(pa_req->data.service);
 	struct bt_iso_base base;
 	struct bt_iso_qos qos;
+	struct pa_timer_data *pa_timer = queue_find(pa_timers,
+				pa_timer_match_adapter, data->adapter);
 
 	DBG("PA Sync done");
 
@@ -1130,8 +1147,8 @@ static void iso_pa_sync_confirm_cb(GIOChannel *io, void *user_data)
 	g_io_channel_unref(data->listen_io);
 	g_io_channel_shutdown(io, TRUE, NULL);
 	data->listen_io = NULL;
-	queue_remove(bcast_pa_requests, pa_req);
-
+	queue_remove(pa_timer->bcast_pa_requests, pa_req);
+	free(pa_req);
 	/* Analyze received BASE data and create remote media endpoints for each
 	 * BIS matching our capabilities
 	 */
@@ -2015,14 +2032,15 @@ static void pa_and_big_sync(struct bap_bcast_pa_req *req);
 
 static gboolean pa_idle_timer(gpointer user_data)
 {
-	struct bap_bcast_pa_req *req = user_data;
+	struct pa_timer_data *pa_timer = user_data;
+	struct bap_bcast_pa_req *req;
 	bool in_progress = FALSE;
 
 	/* Handle timer if no request is in progress */
-	queue_foreach(bcast_pa_requests, check_pa_req_in_progress,
+	queue_foreach(pa_timer->bcast_pa_requests, check_pa_req_in_progress,
 			&in_progress);
 	if (in_progress == FALSE) {
-		req = queue_peek_head(bcast_pa_requests);
+		req = queue_peek_head(pa_timer->bcast_pa_requests);
 		if (req != NULL)
 			switch (req->type) {
 			case BAP_PA_SHORT_REQ:
@@ -2034,6 +2052,14 @@ static gboolean pa_idle_timer(gpointer user_data)
 				pa_and_big_sync(req);
 				break;
 			}
+		else {
+			/* pa_req queue is empty, stop the timer by returning
+			 * FALSE and set the pa_timer_id to 0. This will later
+			 * be used to check if the timer is active.
+			 */
+			pa_timer->pa_timer_id = 0;
+			return FALSE;
+		}
 	}
 
 	return TRUE;
@@ -2043,15 +2069,25 @@ static void setup_accept_io_broadcast(struct bap_data *data,
 					struct bap_setup *setup)
 {
 	struct bap_bcast_pa_req *pa_req = new0(struct bap_bcast_pa_req, 1);
+	struct pa_timer_data *pa_timer = queue_find(pa_timers,
+				pa_timer_match_adapter, data->adapter);
+
+	/* Timer could be stopped if all the short lived requests were treated.
+	 * Check the state of the timer and turn it on so that this requests
+	 * can also be treated.
+	 */
+	if (pa_timer->pa_timer_id == 0)
+		pa_timer->pa_timer_id = g_timeout_add_seconds(
+		PA_IDLE_TIMEOUT, pa_idle_timer, pa_timer);
 
 	/* Add this request to the PA queue.
-	 * We don't need to check the queue here and the timer, as we cannot
-	 * have BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
+	 * We don't need to check the queue here, as we cannot have
+	 * BAP_PA_BIG_SYNC_REQ before a short PA (BAP_PA_SHORT_REQ)
 	 */
 	pa_req->type = BAP_PA_BIG_SYNC_REQ;
 	pa_req->in_progress = FALSE;
 	pa_req->data.setup = setup;
-	queue_push_tail(bcast_pa_requests, pa_req);
+	queue_push_tail(pa_timer->bcast_pa_requests, pa_req);
 }
 
 static void setup_create_ucast_io(struct bap_data *data,
@@ -2880,18 +2916,18 @@ static int bap_bcast_probe(struct btd_service *service)
 	struct btd_adapter *adapter = device_get_adapter(device);
 	struct bap_bcast_pa_req *pa_req =
 			new0(struct bap_bcast_pa_req, 1);
+	struct pa_timer_data *pa_timer =
+			queue_find(pa_timers, pa_timer_match_adapter, adapter);
 
 	if (!btd_adapter_has_exp_feature(adapter, EXP_FEAT_ISO_SOCKET)) {
 		error("BAP requires ISO Socket which is not enabled");
 		return -ENOTSUP;
 	}
 
-	/* First time initialize the queue and start the idle timer */
-	if (bcast_pa_requests == NULL) {
-		bcast_pa_requests = queue_new();
-		pa_timer_id = g_timeout_add_seconds(PA_IDLE_TIMEOUT,
-					pa_idle_timer, NULL);
-	}
+	/* Start the PA timer if it hasn't been started yet */
+	if (pa_timer->pa_timer_id == 0)
+		pa_timer->pa_timer_id = g_timeout_add_seconds(
+		PA_IDLE_TIMEOUT, pa_idle_timer, pa_timer);
 
 	/* Enqueue this device advertisement so that we can do short-lived
 	 */
@@ -2899,17 +2935,38 @@ static int bap_bcast_probe(struct btd_service *service)
 	pa_req->type = BAP_PA_SHORT_REQ;
 	pa_req->in_progress = FALSE;
 	pa_req->data.service = service;
-	queue_push_tail(bcast_pa_requests, pa_req);
+	queue_push_tail(pa_timer->bcast_pa_requests, pa_req);
 
 	return 0;
 }
 
+static bool match_service(const void *data, const void *match_data)
+{
+	struct bap_bcast_pa_req *pa_req = (struct bap_bcast_pa_req *)data;
+
+	if (pa_req->data.service == match_data)
+		return true;
+	return false;
+}
+
 static void bap_bcast_remove(struct btd_service *service)
 {
 	struct btd_device *device = btd_service_get_device(service);
+	struct btd_adapter *adapter = device_get_adapter(device);
+	struct pa_timer_data *pa_timer =
+			queue_find(pa_timers, pa_timer_match_adapter, adapter);
+	struct bap_bcast_pa_req *pa_req;
 	struct bap_data *data;
 	char addr[18];
 
+	/* Remove the corresponding entry from the pa_req queue. Any pa_req that
+	 * are in progress will be stopped by bap_data_remove which calls
+	 * bap_data_free.
+	 */
+	pa_req = queue_remove_if(pa_timer->bcast_pa_requests,
+						match_service, service);
+	if (pa_req)
+		free(pa_req);
 	ba2str(device_get_address(device), addr);
 	DBG("%s", addr);
 
@@ -3021,6 +3078,7 @@ static int bap_adapter_probe(struct btd_profile *p,
 	struct btd_gatt_database *database = btd_adapter_get_database(adapter);
 	struct bap_data *data;
 	char addr[18];
+	struct pa_timer_data *pa_timer;
 
 	ba2str(btd_adapter_get_address(adapter), addr);
 	DBG("%s", addr);
@@ -3055,6 +3113,14 @@ static int bap_adapter_probe(struct btd_profile *p,
 
 	bt_bap_set_user_data(data->bap, adapter);
 	bap_data_set_user_data(data, adapter);
+
+	if (pa_timers == NULL)
+		pa_timers = queue_new();
+	pa_timer = new0(struct pa_timer_data, 1);
+	pa_timer->adapter = adapter;
+	pa_timer->bcast_pa_requests = queue_new();
+	queue_push_tail(pa_timers, pa_timer);
+
 	return 0;
 }
 
@@ -3063,11 +3129,24 @@ static void bap_adapter_remove(struct btd_profile *p,
 {
 	struct bap_data *data = queue_find(sessions, match_data_bap_data,
 						adapter);
+	struct pa_timer_data *pa_timer = queue_find(pa_timers,
+					pa_timer_match_adapter, adapter);
 	char addr[18];
 
 	ba2str(btd_adapter_get_address(adapter), addr);
 	DBG("%s", addr);
 
+	queue_remove_all(pa_timer->bcast_pa_requests,
+					NULL, NULL, free);
+	free(pa_timer->bcast_pa_requests);
+	queue_remove(pa_timers, pa_timer);
+	free(pa_timer);
+
+	if (queue_isempty(pa_timers)) {
+		queue_destroy(pa_timers, NULL);
+		pa_timers = NULL;
+	}
+
 	if (!data) {
 		error("BAP service not handled by profile");
 		return;
-- 
2.40.1