From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lankhorst.se (lankhorst.se [141.105.120.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id ED24B15E8B; Fri, 3 Oct 2025 13:36:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=141.105.120.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759498596; cv=none; b=ALeCich//6taV3CQPtnecWX7fhZp8W3Pu3N+Bru2jsbASwdZFPLgRPu3Y+AOooJVgd8SofIAnN5D+LVta6cyTctcAPi7R89Y5AVrAp8n87LHlQWgfugoXrrKTLDIteZAqYCcny/8E3RQhF1mX5d+A4tRHHmAklZ8103N0qtpF/A= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1759498596; c=relaxed/simple; bh=DZIayvB4sttGe6Rsea8XfTAkQV2J1p/HCN8C89WFSvA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=kfk/xeDxp18Xa+gUfbjEaGWU/wDxOootbOGb4foMayCQ7Iss95Xca+Kk7Cd9tJf9vpn+mUgInXuXYnSvkS84HpnBCJgA6p0rSb1iVSXrE8J5GeCE2YgPquJlSrSSCrNDacAcxXtJWmq8p8E/EnQdovQlfy1Xtz1CNeakVVAc53k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lankhorst.se; spf=pass smtp.mailfrom=lankhorst.se; arc=none smtp.client-ip=141.105.120.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lankhorst.se Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lankhorst.se Message-ID: <799a4e7e-076d-4a9d-888d-69f4e2e89520@lankhorst.se> Date: Fri, 3 Oct 2025 15:29:22 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] devcoredump: Fix circular locking dependency with devcd->mutex. To: linux-kernel@vger.kernel.org Cc: intel-xe@lists.freedesktop.org, Mukesh Ojha , Greg Kroah-Hartman , Johannes Berg , "Rafael J. Wysocki" , Danilo Krummrich , stable@vger.kernel.org, Matthew Brost References: <20250723142416.1020423-1-dev@lankhorst.se> Content-Language: en-US From: Maarten Lankhorst In-Reply-To: <20250723142416.1020423-1-dev@lankhorst.se> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Ping? Den 2025-07-23 kl. 16:24, skrev Maarten Lankhorst: > The original code causes a circular locking dependency found by lockdep. > > ====================================================== > WARNING: possible circular locking dependency detected > 6.16.0-rc6-lgci-xe-xe-pw-151626v3+ #1 Tainted: G S U > ------------------------------------------------------ > xe_fault_inject/5091 is trying to acquire lock: > ffff888156815688 ((work_completion)(&(&devcd->del_wk)->work)){+.+.}-{0:0}, at: __flush_work+0x25d/0x660 > > but task is already holding lock: > > ffff888156815620 (&devcd->mutex){+.+.}-{3:3}, at: dev_coredump_put+0x3f/0xa0 > which lock already depends on the new lock. > the existing dependency chain (in reverse order) is: > -> #2 (&devcd->mutex){+.+.}-{3:3}: > mutex_lock_nested+0x4e/0xc0 > devcd_data_write+0x27/0x90 > sysfs_kf_bin_write+0x80/0xf0 > kernfs_fop_write_iter+0x169/0x220 > vfs_write+0x293/0x560 > ksys_write+0x72/0xf0 > __x64_sys_write+0x19/0x30 > x64_sys_call+0x2bf/0x2660 > do_syscall_64+0x93/0xb60 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > -> #1 (kn->active#236){++++}-{0:0}: > kernfs_drain+0x1e2/0x200 > __kernfs_remove+0xae/0x400 > kernfs_remove_by_name_ns+0x5d/0xc0 > remove_files+0x54/0x70 > sysfs_remove_group+0x3d/0xa0 > sysfs_remove_groups+0x2e/0x60 > device_remove_attrs+0xc7/0x100 > device_del+0x15d/0x3b0 > devcd_del+0x19/0x30 > process_one_work+0x22b/0x6f0 > worker_thread+0x1e8/0x3d0 > kthread+0x11c/0x250 > ret_from_fork+0x26c/0x2e0 > ret_from_fork_asm+0x1a/0x30 > -> #0 ((work_completion)(&(&devcd->del_wk)->work)){+.+.}-{0:0}: > __lock_acquire+0x1661/0x2860 > lock_acquire+0xc4/0x2f0 > __flush_work+0x27a/0x660 > flush_delayed_work+0x5d/0xa0 > dev_coredump_put+0x63/0xa0 > xe_driver_devcoredump_fini+0x12/0x20 [xe] > devm_action_release+0x12/0x30 > release_nodes+0x3a/0x120 > devres_release_all+0x8a/0xd0 > device_unbind_cleanup+0x12/0x80 > device_release_driver_internal+0x23a/0x280 > device_driver_detach+0x14/0x20 > unbind_store+0xaf/0xc0 > drv_attr_store+0x21/0x50 > sysfs_kf_write+0x4a/0x80 > kernfs_fop_write_iter+0x169/0x220 > vfs_write+0x293/0x560 > ksys_write+0x72/0xf0 > __x64_sys_write+0x19/0x30 > x64_sys_call+0x2bf/0x2660 > do_syscall_64+0x93/0xb60 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > other info that might help us debug this: > Chain exists of: (work_completion)(&(&devcd->del_wk)->work) --> kn->active#236 --> &devcd->mutex > Possible unsafe locking scenario: > CPU0 CPU1 > ---- ---- > lock(&devcd->mutex); > lock(kn->active#236); > lock(&devcd->mutex); > lock((work_completion)(&(&devcd->del_wk)->work)); > *** DEADLOCK *** > 5 locks held by xe_fault_inject/5091: > #0: ffff8881129f9488 (sb_writers#5){.+.+}-{0:0}, at: ksys_write+0x72/0xf0 > #1: ffff88810c755078 (&of->mutex#2){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x123/0x220 > #2: ffff8881054811a0 (&dev->mutex){....}-{3:3}, at: device_release_driver_internal+0x55/0x280 > #3: ffff888156815620 (&devcd->mutex){+.+.}-{3:3}, at: dev_coredump_put+0x3f/0xa0 > #4: ffffffff8359e020 (rcu_read_lock){....}-{1:2}, at: __flush_work+0x72/0x660 > stack backtrace: > CPU: 14 UID: 0 PID: 5091 Comm: xe_fault_inject Tainted: G S U 6.16.0-rc6-lgci-xe-xe-pw-151626v3+ #1 PREEMPT_{RT,(lazy)} > Tainted: [S]=CPU_OUT_OF_SPEC, [U]=USER > Hardware name: Micro-Star International Co., Ltd. MS-7D25/PRO Z690-A DDR4(MS-7D25), BIOS 1.10 12/13/2021 > Call Trace: > > dump_stack_lvl+0x91/0xf0 > dump_stack+0x10/0x20 > print_circular_bug+0x285/0x360 > check_noncircular+0x135/0x150 > ? register_lock_class+0x48/0x4a0 > __lock_acquire+0x1661/0x2860 > lock_acquire+0xc4/0x2f0 > ? __flush_work+0x25d/0x660 > ? mark_held_locks+0x46/0x90 > ? __flush_work+0x25d/0x660 > __flush_work+0x27a/0x660 > ? __flush_work+0x25d/0x660 > ? trace_hardirqs_on+0x1e/0xd0 > ? __pfx_wq_barrier_func+0x10/0x10 > flush_delayed_work+0x5d/0xa0 > dev_coredump_put+0x63/0xa0 > xe_driver_devcoredump_fini+0x12/0x20 [xe] > devm_action_release+0x12/0x30 > release_nodes+0x3a/0x120 > devres_release_all+0x8a/0xd0 > device_unbind_cleanup+0x12/0x80 > device_release_driver_internal+0x23a/0x280 > ? bus_find_device+0xa8/0xe0 > device_driver_detach+0x14/0x20 > unbind_store+0xaf/0xc0 > drv_attr_store+0x21/0x50 > sysfs_kf_write+0x4a/0x80 > kernfs_fop_write_iter+0x169/0x220 > vfs_write+0x293/0x560 > ksys_write+0x72/0xf0 > __x64_sys_write+0x19/0x30 > x64_sys_call+0x2bf/0x2660 > do_syscall_64+0x93/0xb60 > ? __f_unlock_pos+0x15/0x20 > ? __x64_sys_getdents64+0x9b/0x130 > ? __pfx_filldir64+0x10/0x10 > ? do_syscall_64+0x1a2/0xb60 > ? clear_bhb_loop+0x30/0x80 > ? clear_bhb_loop+0x30/0x80 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > RIP: 0033:0x76e292edd574 > Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 80 3d d5 ea 0e 00 00 74 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 55 48 89 e5 48 83 ec 20 48 89 > RSP: 002b:00007fffe247a828 EFLAGS: 00000202 ORIG_RAX: 0000000000000001 > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000076e292edd574 > RDX: 000000000000000c RSI: 00006267f6306063 RDI: 000000000000000b > RBP: 000000000000000c R08: 000076e292fc4b20 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000202 R12: 00006267f6306063 > R13: 000000000000000b R14: 00006267e6859c00 R15: 000076e29322a000 > > xe 0000:03:00.0: [drm] Xe device coredump has been deleted. > > Fixes: 01daccf74832 ("devcoredump : Serialize devcd_del work") > Cc: Mukesh Ojha > Cc: Greg Kroah-Hartman > Cc: Johannes Berg > Cc: "Rafael J. Wysocki" > Cc: Danilo Krummrich > Cc: linux-kernel@vger.kernel.org > Cc: # v6.1+ > Signed-off-by: Maarten Lankhorst > Cc: Matthew Brost > --- > drivers/base/devcoredump.c | 136 ++++++++++++++++++++++--------------- > 1 file changed, 83 insertions(+), 53 deletions(-) > > diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c > index 03a39c417dc41..ad4bddde12ccb 100644 > --- a/drivers/base/devcoredump.c > +++ b/drivers/base/devcoredump.c > @@ -23,50 +23,46 @@ struct devcd_entry { > void *data; > size_t datalen; > /* > - * Here, mutex is required to serialize the calls to del_wk work between > - * user/kernel space which happens when devcd is added with device_add() > - * and that sends uevent to user space. User space reads the uevents, > - * and calls to devcd_data_write() which try to modify the work which is > - * not even initialized/queued from devcoredump. > + * There are 2 races for which mutex is required. > * > + * The first race is between device creation and userspace writing to > + * schedule immediately destruction. > * > + * This race is handled by arming the timer before device creation, but > + * when device creation fails the timer still exists. > * > - * cpu0(X) cpu1(Y) > + * To solve this, hold the mutex during device_add(), and set > + * init_completed on success before releasing the mutex. > * > - * dev_coredump() uevent sent to user space > - * device_add() ======================> user space process Y reads the > - * uevents writes to devcd fd > - * which results into writes to > + * That way the timer will never fire until device_add() is called, > + * it will do nothing if init_completed is not set. The timer is also > + * cancelled in that case. > * > - * devcd_data_write() > - * mod_delayed_work() > - * try_to_grab_pending() > - * timer_delete() > - * debug_assert_init() > - * INIT_DELAYED_WORK() > - * schedule_delayed_work() > - * > - * > - * Also, mutex alone would not be enough to avoid scheduling of > - * del_wk work after it get flush from a call to devcd_free() > - * mentioned as below. > - * > - * disabled_store() > - * devcd_free() > - * mutex_lock() devcd_data_write() > - * flush_delayed_work() > - * mutex_unlock() > - * mutex_lock() > - * mod_delayed_work() > - * mutex_unlock() > - * So, delete_work flag is required. > + * The second race involves multiple parallel invocations of devcd_free(), > + * add a deleted flag so only 1 can call the destructor. > */ > struct mutex mutex; > - bool delete_work; > + bool init_completed, deleted; > struct module *owner; > ssize_t (*read)(char *buffer, loff_t offset, size_t count, > void *data, size_t datalen); > void (*free)(void *data); > + /* > + * If nothing interferes and device_add() was returns success, > + * del_wk will destroy the device after the timer fires. > + * > + * Multiple userspace processes can interfere in the working of the timer: > + * - Writing to the coredump will reschedule the timer to run immediately, > + * if still armed. > + * > + * This is handled by using "if (cancel_delayed_work()) { > + * schedule_delayed_work() }", to prevent re-arming after having > + * been previously fired. > + * - Writing to /sys/class/devcoredump/disabled will destroy the > + * coredump synchronously. > + * This is handled by using disable_delayed_work_sync(), and then > + * checking if deleted flag is set with &devcd->mutex held. > + */ > struct delayed_work del_wk; > struct device *failing_dev; > }; > @@ -95,14 +91,27 @@ static void devcd_dev_release(struct device *dev) > kfree(devcd); > } > > +static void __devcd_del(struct devcd_entry *devcd) > +{ > + devcd->deleted = true; > + device_del(&devcd->devcd_dev); > + put_device(&devcd->devcd_dev); > +} > + > static void devcd_del(struct work_struct *wk) > { > struct devcd_entry *devcd; > + bool init_completed; > > devcd = container_of(wk, struct devcd_entry, del_wk.work); > > - device_del(&devcd->devcd_dev); > - put_device(&devcd->devcd_dev); > + /* devcd->mutex serializes against dev_coredumpm_timeout */ > + mutex_lock(&devcd->mutex); > + init_completed = devcd->init_completed; > + mutex_unlock(&devcd->mutex); > + > + if (init_completed) > + __devcd_del(devcd); > } > > static ssize_t devcd_data_read(struct file *filp, struct kobject *kobj, > @@ -122,12 +131,12 @@ static ssize_t devcd_data_write(struct file *filp, struct kobject *kobj, > struct device *dev = kobj_to_dev(kobj); > struct devcd_entry *devcd = dev_to_devcd(dev); > > - mutex_lock(&devcd->mutex); > - if (!devcd->delete_work) { > - devcd->delete_work = true; > - mod_delayed_work(system_wq, &devcd->del_wk, 0); > - } > - mutex_unlock(&devcd->mutex); > + /* > + * Although it's tempting to use mod_delayed work here, > + * that will cause a reschedule if the timer already fired. > + */ > + if (cancel_delayed_work(&devcd->del_wk)) > + schedule_delayed_work(&devcd->del_wk, 0); > > return count; > } > @@ -151,11 +160,21 @@ static int devcd_free(struct device *dev, void *data) > { > struct devcd_entry *devcd = dev_to_devcd(dev); > > + /* > + * To prevent a race with devcd_data_write(), disable work and > + * complete manually instead. > + * > + * We cannot rely on the return value of > + * disable_delayed_work_sync() here, because it might be in the > + * middle of a cancel_delayed_work + schedule_delayed_work pair. > + * > + * devcd->mutex here guards against multiple parallel invocations > + * of devcd_free(). > + */ > + disable_delayed_work_sync(&devcd->del_wk); > mutex_lock(&devcd->mutex); > - if (!devcd->delete_work) > - devcd->delete_work = true; > - > - flush_delayed_work(&devcd->del_wk); > + if (!devcd->deleted) > + __devcd_del(devcd); > mutex_unlock(&devcd->mutex); > return 0; > } > @@ -179,12 +198,10 @@ static ssize_t disabled_show(const struct class *class, const struct class_attri > * put_device() <- last reference > * error = fn(dev, data) devcd_dev_release() > * devcd_free(dev, data) kfree(devcd) > - * mutex_lock(&devcd->mutex); > * > * > * In the above diagram, it looks like disabled_store() would be racing with parallelly > - * running devcd_del() and result in memory abort while acquiring devcd->mutex which > - * is called after kfree of devcd memory after dropping its last reference with > + * running devcd_del() and result in memory abort after dropping its last reference with > * put_device(). However, this will not happens as fn(dev, data) runs > * with its own reference to device via klist_node so it is not its last reference. > * so, above situation would not occur. > @@ -374,7 +391,7 @@ void dev_coredumpm_timeout(struct device *dev, struct module *owner, > devcd->read = read; > devcd->free = free; > devcd->failing_dev = get_device(dev); > - devcd->delete_work = false; > + devcd->deleted = false; > > mutex_init(&devcd->mutex); > device_initialize(&devcd->devcd_dev); > @@ -383,8 +400,14 @@ void dev_coredumpm_timeout(struct device *dev, struct module *owner, > atomic_inc_return(&devcd_count)); > devcd->devcd_dev.class = &devcd_class; > > - mutex_lock(&devcd->mutex); > dev_set_uevent_suppress(&devcd->devcd_dev, true); > + > + /* devcd->mutex prevents devcd_del() completing until init finishes */ > + mutex_lock(&devcd->mutex); > + devcd->init_completed = false; > + INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > + schedule_delayed_work(&devcd->del_wk, timeout); > + > if (device_add(&devcd->devcd_dev)) > goto put_device; > > @@ -401,13 +424,20 @@ void dev_coredumpm_timeout(struct device *dev, struct module *owner, > > dev_set_uevent_suppress(&devcd->devcd_dev, false); > kobject_uevent(&devcd->devcd_dev.kobj, KOBJ_ADD); > - INIT_DELAYED_WORK(&devcd->del_wk, devcd_del); > - schedule_delayed_work(&devcd->del_wk, timeout); > + > + /* > + * Safe to run devcd_del() now that we are done with devcd_dev. > + * Alternatively we could have taken a ref on devcd_dev before > + * dropping the lock. > + */ > + devcd->init_completed = true; > mutex_unlock(&devcd->mutex); > return; > put_device: > - put_device(&devcd->devcd_dev); > mutex_unlock(&devcd->mutex); > + cancel_delayed_work_sync(&devcd->del_wk); > + put_device(&devcd->devcd_dev); > + > put_module: > module_put(owner); > free: