* [PATCH] usb: typec: ucsi: Fix race condition and ordering in port unregistration
@ 2026-07-07 14:17 Andrei Kuchynski
2026-07-08 15:15 ` Benson Leung
0 siblings, 1 reply; 2+ messages in thread
From: Andrei Kuchynski @ 2026-07-07 14:17 UTC (permalink / raw)
To: Heikki Krogerus, Jameson Thies, Benson Leung, linux-usb,
linux-kernel
Cc: Greg Kroah-Hartman, Abhishek Pandit-Subedi, Pooja Katiyar,
Johan Hovold, Hsin-Te Yuan, Myrrh Periwinkle, Linyu Yuan,
Jack Pham, Andrei Kuchynski, stable
A synchronization issue exists during port unregistration where pending
partner work items can race against workqueue destruction, leading to
use-after-free conditions:
cros_ec_ucsi cros_ec_ucsi.3.auto: error -ETIMEDOUT: PPM init failed
BUG: kernel NULL pointer dereference, address: 0000000000000000
RIP: 0010:__queue_work+0x83/0x4a0
Call Trace:
<IRQ>
__cfi_delayed_work_timer_fn+0x10/0x10
run_timer_softirq+0x3b6/0xbd0
sched_clock_cpu+0xc/0x110
irq_exit_rcu+0x18d/0x330
fred_sysvec_apic_timer_interrupt+0x5e/0x80
Fix this by ensuring strict ordering and proper serialization during
teardown:
1. Move ucsi_unregister_partner() to the beginning of the teardown
sequence and protect it under the connector mutex lock.
2. Ensure all pending partner tasks are explicitly flushed and finished
before the workqueue is destroyed.
3. Switch from mod_delayed_work() to a cancel_delayed_work() and
queue_delayed_work() sequence. This guarantees that items currently marked
as pending won't be scheduled an additional time, preventing a double
release of resources which leads to the following crash:
Oops: general protection fault, probably for non-canonical address
0xdead000000000122: 0000 [#1] SMP NOPTI
Workqueue: cros_ec_ucsi.3.auto-con2 ucsi_poll_worker
RIP: 0010:ucsi_poll_worker+0x65/0x1e0
Call Trace:
<TASK>
process_scheduled_works+0x218/0x6d0
worker_thread+0x188/0x3f0
__cfi_worker_thread+0x10/0x10
kthread+0x226/0x2a0
To ensure these rules are applied identically across both the normal
teardown and the ucsi_init() error paths, consolidate the cleanup logic
into a new helper, ucsi_unregister_port().
Cc: stable@vger.kernel.org
Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
Fixes: b13abcb7ddd8 ("usb: typec: ucsi: Fix NULL pointer access")
Fixes: fac4b8633fd6 ("usb: ucsi: Ensure connector delayed work items are flushed")
Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
---
drivers/usb/typec/ucsi/ucsi.c | 82 +++++++++++++++++------------------
1 file changed, 39 insertions(+), 43 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 92166a3725b16..d9668ed7c80ea 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1845,6 +1845,42 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
return ret;
}
+static void ucsi_unregister_port(struct ucsi_connector *con)
+{
+ struct ucsi_work *uwork;
+
+ if (con->wq) {
+ mutex_lock(&con->lock);
+ ucsi_unregister_partner(con);
+ /*
+ * queue delayed items immediately so they can execute
+ * and free themselves before the wq is destroyed
+ */
+ list_for_each_entry(uwork, &con->partner_tasks, node) {
+ if (cancel_delayed_work(&uwork->work))
+ queue_delayed_work(con->wq, &uwork->work, 0);
+ }
+ mutex_unlock(&con->lock);
+
+ destroy_workqueue(con->wq);
+ con->wq = NULL;
+ } else {
+ ucsi_unregister_partner(con);
+ }
+
+ ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
+ ucsi_unregister_port_psy(con);
+
+ usb_power_delivery_unregister_capabilities(con->port_sink_caps);
+ con->port_sink_caps = NULL;
+ usb_power_delivery_unregister_capabilities(con->port_source_caps);
+ con->port_source_caps = NULL;
+ usb_power_delivery_unregister(con->pd);
+ con->pd = NULL;
+ typec_unregister_port(con->port);
+ con->port = NULL;
+}
+
static u64 ucsi_get_supported_notifications(struct ucsi *ucsi)
{
u16 features = ucsi->cap.features;
@@ -1971,22 +2007,8 @@ static int ucsi_init(struct ucsi *ucsi)
for (i = 0; i < ucsi->cap.num_connectors; i++)
lockdep_unregister_key(&connector[i].lock_key);
- for (con = connector; con->port; con++) {
- if (con->wq)
- destroy_workqueue(con->wq);
- ucsi_unregister_partner(con);
- ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
- ucsi_unregister_port_psy(con);
-
- usb_power_delivery_unregister_capabilities(con->port_sink_caps);
- con->port_sink_caps = NULL;
- usb_power_delivery_unregister_capabilities(con->port_source_caps);
- con->port_source_caps = NULL;
- usb_power_delivery_unregister(con->pd);
- con->pd = NULL;
- typec_unregister_port(con->port);
- con->port = NULL;
- }
+ for (con = connector; con->port; con++)
+ ucsi_unregister_port(con);
kfree(connector);
err_reset:
memset(&ucsi->cap, 0, sizeof(ucsi->cap));
@@ -2194,33 +2216,7 @@ void ucsi_unregister(struct ucsi *ucsi)
for (i = 0; i < ucsi->cap.num_connectors; i++) {
cancel_work_sync(&ucsi->connector[i].work);
-
- if (ucsi->connector[i].wq) {
- struct ucsi_work *uwork;
-
- mutex_lock(&ucsi->connector[i].lock);
- /*
- * queue delayed items immediately so they can execute
- * and free themselves before the wq is destroyed
- */
- list_for_each_entry(uwork, &ucsi->connector[i].partner_tasks, node)
- mod_delayed_work(ucsi->connector[i].wq, &uwork->work, 0);
- mutex_unlock(&ucsi->connector[i].lock);
- destroy_workqueue(ucsi->connector[i].wq);
- }
-
- ucsi_unregister_partner(&ucsi->connector[i]);
- ucsi_unregister_altmodes(&ucsi->connector[i],
- UCSI_RECIPIENT_CON);
- ucsi_unregister_port_psy(&ucsi->connector[i]);
-
- usb_power_delivery_unregister_capabilities(ucsi->connector[i].port_sink_caps);
- ucsi->connector[i].port_sink_caps = NULL;
- usb_power_delivery_unregister_capabilities(ucsi->connector[i].port_source_caps);
- ucsi->connector[i].port_source_caps = NULL;
- usb_power_delivery_unregister(ucsi->connector[i].pd);
- ucsi->connector[i].pd = NULL;
- typec_unregister_port(ucsi->connector[i].port);
+ ucsi_unregister_port(&ucsi->connector[i]);
lockdep_unregister_key(&ucsi->connector[i].lock_key);
}
--
2.55.0.rc2.803.g1fd1e6609c-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] usb: typec: ucsi: Fix race condition and ordering in port unregistration
2026-07-07 14:17 [PATCH] usb: typec: ucsi: Fix race condition and ordering in port unregistration Andrei Kuchynski
@ 2026-07-08 15:15 ` Benson Leung
0 siblings, 0 replies; 2+ messages in thread
From: Benson Leung @ 2026-07-08 15:15 UTC (permalink / raw)
To: Andrei Kuchynski
Cc: Heikki Krogerus, Jameson Thies, Benson Leung, linux-usb,
linux-kernel, Greg Kroah-Hartman, Abhishek Pandit-Subedi,
Pooja Katiyar, Johan Hovold, Hsin-Te Yuan, Myrrh Periwinkle,
Linyu Yuan, Jack Pham, stable
[-- Attachment #1: Type: text/plain, Size: 6356 bytes --]
On Tue, Jul 07, 2026 at 02:17:36PM +0000, Andrei Kuchynski wrote:
> A synchronization issue exists during port unregistration where pending
> partner work items can race against workqueue destruction, leading to
> use-after-free conditions:
>
> cros_ec_ucsi cros_ec_ucsi.3.auto: error -ETIMEDOUT: PPM init failed
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> RIP: 0010:__queue_work+0x83/0x4a0
> Call Trace:
> <IRQ>
> __cfi_delayed_work_timer_fn+0x10/0x10
> run_timer_softirq+0x3b6/0xbd0
> sched_clock_cpu+0xc/0x110
> irq_exit_rcu+0x18d/0x330
> fred_sysvec_apic_timer_interrupt+0x5e/0x80
>
> Fix this by ensuring strict ordering and proper serialization during
> teardown:
>
> 1. Move ucsi_unregister_partner() to the beginning of the teardown
> sequence and protect it under the connector mutex lock.
> 2. Ensure all pending partner tasks are explicitly flushed and finished
> before the workqueue is destroyed.
> 3. Switch from mod_delayed_work() to a cancel_delayed_work() and
> queue_delayed_work() sequence. This guarantees that items currently marked
> as pending won't be scheduled an additional time, preventing a double
> release of resources which leads to the following crash:
>
> Oops: general protection fault, probably for non-canonical address
> 0xdead000000000122: 0000 [#1] SMP NOPTI
> Workqueue: cros_ec_ucsi.3.auto-con2 ucsi_poll_worker
> RIP: 0010:ucsi_poll_worker+0x65/0x1e0
> Call Trace:
> <TASK>
> process_scheduled_works+0x218/0x6d0
> worker_thread+0x188/0x3f0
> __cfi_worker_thread+0x10/0x10
> kthread+0x226/0x2a0
>
> To ensure these rules are applied identically across both the normal
> teardown and the ucsi_init() error paths, consolidate the cleanup logic
> into a new helper, ucsi_unregister_port().
>
> Cc: stable@vger.kernel.org
> Fixes: b9aa02ca39a4 ("usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking")
> Fixes: b13abcb7ddd8 ("usb: typec: ucsi: Fix NULL pointer access")
> Fixes: fac4b8633fd6 ("usb: ucsi: Ensure connector delayed work items are flushed")
> Signed-off-by: Andrei Kuchynski <akuchynski@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 82 +++++++++++++++++------------------
> 1 file changed, 39 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 92166a3725b16..d9668ed7c80ea 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1845,6 +1845,42 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
> return ret;
> }
>
> +static void ucsi_unregister_port(struct ucsi_connector *con)
> +{
> + struct ucsi_work *uwork;
> +
> + if (con->wq) {
> + mutex_lock(&con->lock);
> + ucsi_unregister_partner(con);
> + /*
> + * queue delayed items immediately so they can execute
> + * and free themselves before the wq is destroyed
> + */
> + list_for_each_entry(uwork, &con->partner_tasks, node) {
> + if (cancel_delayed_work(&uwork->work))
> + queue_delayed_work(con->wq, &uwork->work, 0);
> + }
> + mutex_unlock(&con->lock);
> +
> + destroy_workqueue(con->wq);
> + con->wq = NULL;
> + } else {
> + ucsi_unregister_partner(con);
> + }
> +
> + ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> + ucsi_unregister_port_psy(con);
> +
> + usb_power_delivery_unregister_capabilities(con->port_sink_caps);
> + con->port_sink_caps = NULL;
> + usb_power_delivery_unregister_capabilities(con->port_source_caps);
> + con->port_source_caps = NULL;
> + usb_power_delivery_unregister(con->pd);
> + con->pd = NULL;
> + typec_unregister_port(con->port);
> + con->port = NULL;
> +}
> +
> static u64 ucsi_get_supported_notifications(struct ucsi *ucsi)
> {
> u16 features = ucsi->cap.features;
> @@ -1971,22 +2007,8 @@ static int ucsi_init(struct ucsi *ucsi)
> for (i = 0; i < ucsi->cap.num_connectors; i++)
> lockdep_unregister_key(&connector[i].lock_key);
>
> - for (con = connector; con->port; con++) {
> - if (con->wq)
> - destroy_workqueue(con->wq);
> - ucsi_unregister_partner(con);
> - ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
> - ucsi_unregister_port_psy(con);
> -
> - usb_power_delivery_unregister_capabilities(con->port_sink_caps);
> - con->port_sink_caps = NULL;
> - usb_power_delivery_unregister_capabilities(con->port_source_caps);
> - con->port_source_caps = NULL;
> - usb_power_delivery_unregister(con->pd);
> - con->pd = NULL;
> - typec_unregister_port(con->port);
> - con->port = NULL;
> - }
> + for (con = connector; con->port; con++)
> + ucsi_unregister_port(con);
> kfree(connector);
> err_reset:
> memset(&ucsi->cap, 0, sizeof(ucsi->cap));
> @@ -2194,33 +2216,7 @@ void ucsi_unregister(struct ucsi *ucsi)
>
> for (i = 0; i < ucsi->cap.num_connectors; i++) {
> cancel_work_sync(&ucsi->connector[i].work);
> -
> - if (ucsi->connector[i].wq) {
> - struct ucsi_work *uwork;
> -
> - mutex_lock(&ucsi->connector[i].lock);
> - /*
> - * queue delayed items immediately so they can execute
> - * and free themselves before the wq is destroyed
> - */
> - list_for_each_entry(uwork, &ucsi->connector[i].partner_tasks, node)
> - mod_delayed_work(ucsi->connector[i].wq, &uwork->work, 0);
> - mutex_unlock(&ucsi->connector[i].lock);
> - destroy_workqueue(ucsi->connector[i].wq);
> - }
> -
> - ucsi_unregister_partner(&ucsi->connector[i]);
> - ucsi_unregister_altmodes(&ucsi->connector[i],
> - UCSI_RECIPIENT_CON);
> - ucsi_unregister_port_psy(&ucsi->connector[i]);
> -
> - usb_power_delivery_unregister_capabilities(ucsi->connector[i].port_sink_caps);
> - ucsi->connector[i].port_sink_caps = NULL;
> - usb_power_delivery_unregister_capabilities(ucsi->connector[i].port_source_caps);
> - ucsi->connector[i].port_source_caps = NULL;
> - usb_power_delivery_unregister(ucsi->connector[i].pd);
> - ucsi->connector[i].pd = NULL;
> - typec_unregister_port(ucsi->connector[i].port);
> + ucsi_unregister_port(&ucsi->connector[i]);
> lockdep_unregister_key(&ucsi->connector[i].lock_key);
> }
>
> --
> 2.55.0.rc2.803.g1fd1e6609c-goog
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-07-08 15:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-07 14:17 [PATCH] usb: typec: ucsi: Fix race condition and ordering in port unregistration Andrei Kuchynski
2026-07-08 15:15 ` Benson Leung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox