Re: REGRESSION: [PATCH] usb: typec: ucsi: Fix race condition and ordering in port unregistration
"Borah, Chaitanya Kumar" <[email protected]>
| Newsgroups | org.freedesktop.lists.intel-gfx,org.freedesktop.lists.intel-xe,org.kernel.vger.linux-kernel,org.kernel.vger.linux-usb,org.kernel.vger.stable |
|---|---|
| Message-ID | <[email protected]> |
On 7/16/2026 7:11 PM, Andrei Kuchynski wrote: > On Thu, Jul 16, 2026 at 3:23 PM Borah, Chaitanya Kumar > <[email protected]> wrote: >> >> >> >> On 7/16/2026 2:45 PM, Andrei Kuchynski wrote: >>> On Thu, Jul 16, 2026 at 10:28 AM Borah, Chaitanya Kumar >>> <[email protected]> wrote: >>>> >>>> Hello Andrei, >>>> >>>> On 7/7/2026 7:47 PM, 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(). >>>> >>>> >>>> This seems to be causing regression in our linux-next CI. [1] >>>> >>>> <6>[ 18.008557] ------------[ cut here ]------------ >>>> <4>[ 18.013166] DEBUG_LOCKS_WARN_ON(1) >>>> <4>[ 18.013167] WARNING: kernel/locking/lockdep.c:238 at >>>> __lock_acquire+0xa59/0x2750, CPU#1: kworker/u89:1/172 >>>> ... >>>> <4>[ 18.159950] Hardware name: Intel Corporation Meteor Lake Client >>>> Platform/MTL-P DDR5 SODIMM SBS RVP, BIOS >>>> MTLPFWI1.R00.3471.D89.2401091901 01/09/2024 >>>> <4>[ 18.173125] Workqueue: events_dfl_long ucsi_init_work [typec_ucsi] >>>> <4>[ 18.179257] RIP: 0010:__lock_acquire+0xa60/0x2750 >>>> <4>[ 18.183927] Code: 44 8b 5d a8 85 c0 0f 84 98 fd ff ff 44 8b 35 07 >>>> 0b 72 02 45 85 f6 0f 85 88 fd ff ff 48 8d 3d 27 25 73 02 48 c7 c6 1b 51 >>>> 27 83 <67> 48 0f b9 3a 31 c0 44 8b 5d a8 44 8b 4d b0 e9 c6 f7 ff ff 31 d2 >>>> ... >>>> <4>[ 18.263886] PKRU: 55555554 >>>> <4>[ 18.266583] Call Trace: >>>> <4>[ 18.269014] <TASK> >>>> <4>[ 18.271110] ? mark_held_locks+0x46/0x90 >>>> <4>[ 18.275007] lock_acquire+0xd9/0x2e0 >>>> <4>[ 18.278562] ? ucsi_unregister_port+0x36/0x140 [typec_ucsi] >>>> <4>[ 18.284096] __mutex_lock+0xb2/0x1020 >>>> <4>[ 18.287735] ? ucsi_unregister_port+0x36/0x140 [typec_ucsi] >>>> <4>[ 18.293265] ? ucsi_unregister_port+0x36/0x140 [typec_ucsi] >>>> <4>[ 18.298797] ? synchronize_rcu_expedited+0x2b2/0x300 >>>> <4>[ 18.303724] ? __pfx_autoremove_wake_function+0x10/0x10 >>>> <4>[ 18.308912] mutex_lock_nested+0x1b/0x30 >>>> <4>[ 18.312806] ? mutex_lock_nested+0x1b/0x30 >>>> <4>[ 18.316878] ucsi_unregister_port+0x36/0x140 [typec_ucsi] >>>> <4>[ 18.322238] ucsi_init_work+0x66d/0xc10 [typec_ucsi] >>>> <4>[ 18.327167] process_one_work+0x2ad/0x8b0 >>>> <4>[ 18.331158] worker_thread+0x200/0x3f0 >>>> <4>[ 18.334886] ? __pfx_worker_thread+0x10/0x10 >>>> <4>[ 18.339128] kthread+0x10d/0x150 >>>> <4>[ 18.342339] ? __pfx_kthread+0x10/0x10 >>>> <4>[ 18.346067] ret_from_fork+0x3bd/0x470 >>>> <4>[ 18.349795] ? __pfx_kthread+0x10/0x10 >>>> <4>[ 18.353522] ret_from_fork_asm+0x1a/0x30 >>>> <4>[ 18.357420] </TASK> >>>> >>>> Detailed log can be seen found in [2] >>>> >>>> We confirmed that reverting the patch solves the issue. >>>> >>>> Could you please check why the patch causes this regression and provide >>>> a fix if necessary? >>>> >>>> Regards >>>> Chaitanya >>>> >>>> [1] https://intel-gfx-ci.01.org/tree/linux-next/combined-alt.html? >>>> [2] >>>> https://intel-gfx-ci.01.org/tree/linux-next/next-20260714/bat-mtlp-9/boot0.txt >>>> >>>> -- Bisect Logs -- >>>> >>>> git bisect start >>>> # status: waiting for both good and bad commits >>>> # good: [5f1b513690edf51727e6928b97705b6437f9a98e] Merge remote-tracking >>>> branch 'spi/for-7.3' into spi-next >>>> git bisect good 5f1b513690edf51727e6928b97705b6437f9a98e >>>> # status: waiting for bad commit, 1 good commit known >>>> # bad: [49362394dad7df66c274c867a271394c10ca2bb8] Add linux-next >>>> specific files for 20260713 >>>> git bisect bad 49362394dad7df66c274c867a271394c10ca2bb8 >>>> # bad: [dffddeb046466ed57038295bde28b42b7506a12b] Merge branch >>>> 'nand/next' of https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git >>>> git bisect bad dffddeb046466ed57038295bde28b42b7506a12b >>>> # bad: [d154d0766bf3fe830e1bf98fb2d8648294a4a8da] Merge branch >>>> 'for-next' of >>>> https://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap.git >>>> git bisect bad d154d0766bf3fe830e1bf98fb2d8648294a4a8da >>>> # bad: [671b0fd929bec9ebf863cb376baf9b9d1226a510] Merge branch 'master' >>>> of https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git >>>> git bisect bad 671b0fd929bec9ebf863cb376baf9b9d1226a510 >>>> # good: [f4fb100039e96211609dfc44fb24b9e4a8a0f2f9] Merge tag >>>> 's390-7.2-4' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux >>>> git bisect good f4fb100039e96211609dfc44fb24b9e4a8a0f2f9 >>>> # good: [c296d513e034df93ddb230ad1c7a118d6b58750c] Merge branch >>>> 'arm/fixes' of https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git >>>> git bisect good c296d513e034df93ddb230ad1c7a118d6b58750c >>>> # good: [de53c0a584f08f31eb957f8c849a59fb33b27fa8] Merge branch 'for-rc' >>>> of https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git >>>> git bisect good de53c0a584f08f31eb957f8c849a59fb33b27fa8 >>>> # bad: [a6969de5a038bc08a948ada52bd7199a8dfa6cec] Merge branch >>>> 'usb-linus' of >>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git >>>> git bisect bad a6969de5a038bc08a948ada52bd7199a8dfa6cec >>>> # good: [15eab2dd63a97587405ab6bfaad9728d5b35b5fe] Merge tag >>>> 'asoc-fix-v7.2-rc2' of >>>> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into for-linus >>>> git bisect good 15eab2dd63a97587405ab6bfaad9728d5b35b5fe >>>> # bad: [c2e819be6a5c7f34344926b4bd7e3dfca58cf48a] usb: gadget: printer: >>>> fix infinite loop in printer_read() >>>> git bisect bad c2e819be6a5c7f34344926b4bd7e3dfca58cf48a >>>> # good: [29a142d3e8b35ebc9e0bcc78f4bc26c9b6a9ac0b] USB: gadget: >>>> snps-udc: fix device name leak on probe failure >>>> git bisect good 29a142d3e8b35ebc9e0bcc78f4bc26c9b6a9ac0b >>>> # good: [1febec7e47cdcd01f43fb0211094e3010474666e] usb: gadget: f_ncm: >>>> validate datagram bounds in ncm_unwrap_ntb() >>>> git bisect good 1febec7e47cdcd01f43fb0211094e3010474666e >>>> # bad: [0583f2fbf8f86ae3a0ce054f96783dd83e65d9bb] usb: gadget: udc: bdc: >>>> free IRQ and drain func_wake_notify before teardown >>>> git bisect bad 0583f2fbf8f86ae3a0ce054f96783dd83e65d9bb >>>> # bad: [7aa7d4bf9d3fa9a6a47b640ad103ab433b7ff261] usb: typec: ucsi: Fix >>>> race condition and ordering in port unregistration >>>> git bisect bad 7aa7d4bf9d3fa9a6a47b640ad103ab433b7ff261 >>>> # first bad commit: [7aa7d4bf9d3fa9a6a47b640ad103ab433b7ff261] usb: >>>> typec: ucsi: Fix race condition and ordering in port unregistration >>>>> >>>>> Cc: [email protected] >>>>> 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 <[email protected]> >>>>> --- >>>>> 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); >>> >>> Hi Chaitanya, >>> Thank you for spotting this! >>> >>> Could you verify if calling lockdep_unregister_key(&connector[i].lock_key) >>> after ucsi_unregister_port(con) resolves the issue? >>> >>> for (con = connector; con->port; con++) >>> ucsi_unregister_port(con); >>> for (i = 0; i < ucsi->cap.num_connectors; i++) >>> lockdep_unregister_key(&connector[i].lock_key); >>> >>> I will also try testing this on my side. >> >> Thank you Andrei for the patch. It seems to work for us. >> > > Hi Chaitanya, > > Thanks for the testing! > I will prepare a fix or maybe V2 of this patch. > > Could you please share your configuration details? Specifically, do you > have any of the following enabled? > CONFIG_DEBUG_LOCKDEP > CONFIG_LOCK_STAT > CONFIG_LOCKDEP_SMALL > CONFIG_DEBUG_LOCKDEP > CONFIG_PROVE_LOCKING > > I am still not able to reproduce it. I hope this is accessible. https://intel-gfx-ci.01.org/tree/linux-next/next-20260713/kconfig.txt We also see this issue only in one of the CI machines. So it might not be the config. == Chaitanya > > Thanks, > Andrei