Hi Alan,
On 08.08.2022 22:26, Alan Stern wrote:
> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB:
>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it
>> fixes the issue by introducing another one. It doesn't look very
>> probable, but it would be nice to fix it to make the lock dependency
>> checker happy.
> Indeed.
>
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.19.0-rc7+ #12510 Not tainted
>> ------------------------------------------------------
>> udevadm/312 is trying to acquire lock:
>> ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0
>>
>> but task is already holding lock:
>> ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0
>>
>> which lock already depends on the new lock.
>>
>>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #3 (kn->active#4){++++}-{0:0}:
>> lock_acquire+0x68/0x84
>> __kernfs_remove+0x268/0x380
>> kernfs_remove_by_name_ns+0x58/0xac
>> sysfs_remove_file_ns+0x18/0x24
>> device_del+0x15c/0x440
>> device_link_drop_managed+0xa8/0xe0
>> device_links_driver_bound+0x1b8/0x230
>> driver_bound+0x68/0xc0
>> really_probe.part.0+0x1f8/0x2ac
>> __driver_probe_device+0x98/0x144
>> driver_probe_device+0xac/0x14c
>> __driver_attach+0x104/0x1b0
>> bus_for_each_dev+0x70/0xd0
>> driver_attach+0x24/0x30
>> bus_add_driver+0x154/0x204
>> driver_register+0x78/0x130
>> __platform_driver_register+0x28/0x34
>> simple_pm_bus_driver_init+0x1c/0x28
>> do_one_initcall+0x74/0x400
>> kernel_init_freeable+0x2f4/0x37c
>> kernel_init+0x28/0x130
>> ret_from_fork+0x10/0x20
>>
>> -> #2 (device_links_lock){+.+.}-{3:3}:
>> lock_acquire+0x68/0x84
>> __mutex_lock+0x9c/0x430
>> mutex_lock_nested+0x38/0x64
>> device_link_remove+0x3c/0xa0
>> _regulator_put.part.0+0x168/0x190
>> regulator_put+0x3c/0x54
>> devm_regulator_release+0x14/0x20
>> release_nodes+0x5c/0x90
>> devres_release_all+0x8c/0xe0
>> device_unbind_cleanup+0x18/0x70
>> really_probe.part.0+0x174/0x2ac
>> __driver_probe_device+0x98/0x144
>> driver_probe_device+0xac/0x14c
>> __device_attach_driver+0xb8/0x120
>> bus_for_each_drv+0x78/0xd0
>> __device_attach_async_helper+0xb0/0xd4
>> async_run_entry_fn+0x34/0xd0
>> process_one_work+0x288/0x6bc
>> worker_thread+0x74/0x450
>> kthread+0x118/0x11c
>> ret_from_fork+0x10/0x20
>>
>> -> #1 (regulator_list_mutex){+.+.}-{3:3}:
>> lock_acquire+0x68/0x84
>> __mutex_lock+0x9c/0x430
>> mutex_lock_nested+0x38/0x64
>> regulator_lock_dependent+0x54/0x284
>> regulator_enable+0x34/0x80
>> phy_power_on+0x24/0x130
>> __dwc2_lowlevel_hw_enable+0x100/0x130
>> dwc2_lowlevel_hw_enable+0x18/0x40
>> dwc2_hsotg_udc_start+0x6c/0x2f0
>> gadget_bind_driver+0x124/0x1f4
>> really_probe.part.0+0x9c/0x2ac
>> __driver_probe_device+0x98/0x144
>> driver_probe_device+0xac/0x14c
>> __device_attach_driver+0xb8/0x120
>> bus_for_each_drv+0x78/0xd0
>> __device_attach+0xa8/0x1c0
>> device_initial_probe+0x14/0x20
>> bus_probe_device+0x9c/0xa4
>> device_add+0x3a0/0x890
>> usb_add_gadget+0x170/0x200
>> usb_add_gadget_udc+0x94/0xd4
>> dwc2_driver_probe+0x580/0x78c
>> platform_probe+0x68/0xe0
>> really_probe.part.0+0x9c/0x2ac
>> __driver_probe_device+0x98/0x144
>> driver_probe_device+0xac/0x14c
>> __device_attach_driver+0xb8/0x120
>> bus_for_each_drv+0x78/0xd0
>> __device_attach+0xa8/0x1c0
>> device_initial_probe+0x14/0x20
>> bus_probe_device+0x9c/0xa4
>> device_add+0x3a0/0x890
>> of_device_add+0x48/0x6c
>> of_platform_device_create_pdata+0x98/0x100
>> of_platform_bus_create+0x17c/0x37c
>> of_platform_populate+0x58/0xec
>> dwc3_meson_g12a_probe+0x314/0x5d0
>> platform_probe+0x68/0xe0
>> really_probe.part.0+0x9c/0x2ac
>> __driver_probe_device+0x98/0x144
>> driver_probe_device+0xac/0x14c
>> __device_attach_driver+0xb8/0x120
>> bus_for_each_drv+0x78/0xd0
>> __device_attach+0xa8/0x1c0
>> device_initial_probe+0x14/0x20
>> bus_probe_device+0x9c/0xa4
>> deferred_probe_work_func+0x88/0xc4
>> process_one_work+0x288/0x6bc
>> worker_thread+0x74/0x450
>> kthread+0x118/0x11c
>> ret_from_fork+0x10/0x20
>>
>> -> #0 (udc_lock){+.+.}-{3:3}:
>> __lock_acquire+0x1298/0x20cc
>> lock_acquire.part.0+0xe0/0x230
>> lock_acquire+0x68/0x84
>> __mutex_lock+0x9c/0x430
>> mutex_lock_nested+0x38/0x64
>> usb_udc_uevent+0x54/0xe0
>> dev_uevent+0xb8/0x1ec
>> uevent_show+0x8c/0x114
>> dev_attr_show+0x20/0x60
>> sysfs_kf_seq_show+0xa8/0x120
>> kernfs_seq_show+0x2c/0x40
>> seq_read_iter+0x1bc/0x4b0
>> kernfs_fop_read_iter+0x140/0x1d0
>> new_sync_read+0xd4/0x150
>> vfs_read+0x190/0x1dc
>> ksys_read+0x68/0xfc
>> __arm64_sys_read+0x20/0x30
>> invoke_syscall+0x48/0x114
>> el0_svc_common.constprop.0+0x60/0x11c
>> do_el0_svc_compat+0x1c/0x50
>> el0_svc_compat+0x58/0x100
>> el0t_32_sync_handler+0x90/0x140
>> el0t_32_sync+0x190/0x194
>>
>> other info that might help us debug this:
>>
>> Chain exists of:
>> udc_lock --> device_links_lock --> kn->active#4
> Peter, shouldn't this say:
>
> udc_lock --> regulator_list_mutex --> device_links_lock --> kn->active#4
>
> ? Is that a bug in the lockdep reporting?
>
> It looks like this is a bad interaction between the udc_lock and the
> sysfs/kernfs locking. If a lock such as udc_lock is held (or is part of
> a chain) while a sysfs file is removed, the file's ->show or ->store
> routine better not acquire that lock.
>
> I suspect the problem is that udc_lock is held for too long. Probably it
> should be released during the calls to udc->driver->bind and
> udc->driver->unbind.
>
> Getting this right will require some careful study. Marek, if I send you
> a patch later, will you be able to test it?
Yes, sure. I'm available till 12th August, then I'm going for holidays.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
