Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 21 Jun 2022 21:34:26 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "zhangwensheng (E)" <zhangwensheng5@huawei.com>
Cc:     rafael@kernel.org, linux-kernel@vger.kernel.org, yukuai3@huawei.com
Subject: Re: [PATCH -next] driver core: fix deadlock in __driver_attach
Message-ID: <YrIdQlkfS29fbAHx@kroah.com>
References: <20220608094355.3298420-1-zhangwensheng5@huawei.com>
 <YqNL6NPgP+cLOy/I@kroah.com>
 <4e5e9276-e32d-0903-1f4e-20880cc72d82@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4e5e9276-e32d-0903-1f4e-20880cc72d82@huawei.com>
Precedence: bulk


A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Thu, Jun 16, 2022 at 04:00:58PM +0800, zhangwensheng (E) wrote:
> sorry that I didn't see your reply.
> it is real not potential, I have triggered this problem successfully and
> proven that this change can fix it.
> 
> stack like commit b232b02bf3c2 ("driver core: fix deadlock in
> __device_attach").
> list below:
> ??? In __driver_attach function, The lock holding logic is as follows:
> ??? ...
> ??? __driver_attach
> ??? if (driver_allows_async_probing(drv))
> ????? device_lock(dev)????? // get lock dev
> ??????? async_schedule_dev(__driver_attach_async_helper, dev); // func
> ????????? async_schedule_node
> ??????????? async_schedule_node_domain(func)
> ????????????? entry = kzalloc(sizeof(struct async_entry), GFP_ATOMIC);
> ????????????? /* when fail or work limit, sync to execute func, but
> ???????????????? __driver_attach_async_helper will get lock dev as
> ???????????????? will, which will lead to A-A deadlock.? */
> ????????????? if (!entry || atomic_read(&entry_count) > MAX_WORK) {
> ??????????????? func;
> ????????????? else
> ??????????????? queue_work_node(node, system_unbound_wq, &entry->work)
> ????? device_unlock(dev)
> 
> ??? As above show, when it is allowed to do async probes, because of
> ??? out of memory or work limit, async work is not be allowed, to do
> ??? sync execute instead. it will lead to A-A deadlock because of
> ??? __driver_attach_async_helper getting lock dev.
> 
> ??? Because it's logic is same as commit b232b02bf3c2 ("driver core: fix
> deadlock
> ??? in __device_attach"),? I simplify the description.
> 
> 
> Reproduce:
> and it can be reproduce by make the condition
> (if (!entry || atomic_read(&entry_count) > MAX_WORK)) untenable, like below:
> 
> [? 370.785650] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables
> this message.
> [? 370.787154] task:swapper/0?????? state:D stack:??? 0 pid:??? 1 ppid:????
> 0 flags:0x00004000
> [? 370.788865] Call Trace:
> [? 370.789374]? <TASK>
> [? 370.789841]? __schedule+0x482/0x1050
> [? 370.790613]? schedule+0x92/0x1a0
> [? 370.791290]? schedule_preempt_disabled+0x2c/0x50
> [? 370.792256]? __mutex_lock.isra.0+0x757/0xec0
> [? 370.793158]? __mutex_lock_slowpath+0x1f/0x30
> [? 370.794079]? mutex_lock+0x50/0x60
> [? 370.794795]? __device_driver_lock+0x2f/0x70
> [? 370.795677]? ? driver_probe_device+0xd0/0xd0
> [? 370.796576]? __driver_attach_async_helper+0x1d/0xd0
> [? 370.797318]? ? driver_probe_device+0xd0/0xd0
> [? 370.797957]? async_schedule_node_domain+0xa5/0xc0
> [? 370.798652]? async_schedule_node+0x19/0x30
> [? 370.799243]? __driver_attach+0x246/0x290
> [? 370.799828]? ? driver_allows_async_probing+0xa0/0xa0
> [? 370.800548]? bus_for_each_dev+0x9d/0x130
> [? 370.801132]? driver_attach+0x22/0x30
> [? 370.801666]? bus_add_driver+0x290/0x340
> [? 370.802246]? driver_register+0x88/0x140
> [? 370.802817]? ? virtio_scsi_init+0x116/0x116
> [? 370.803425]? scsi_register_driver+0x1a/0x30
> [? 370.804057]? init_sd+0x184/0x226
> [? 370.804533]? do_one_initcall+0x71/0x3a0
> [? 370.805107]? kernel_init_freeable+0x39a/0x43a
> [? 370.805759]? ? rest_init+0x150/0x150
> [? 370.806283]? kernel_init+0x26/0x230
> [? 370.806799]? ret_from_fork+0x1f/0x30
> 
> And my change can fix it.

Ok, please put that type of information in the changelog text.

thanks,

greg k-h