linux-kernel - Re: [PATCH] debugfs: fix potential infinite loop in debugfs_remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 Nov 2019 11:20:41 +0800
From:   "yukuai (C)" <yukuai3@...wei.com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <oleg@...hat.com>, <jack@...e.cz>, <linux-kernel@...r.kernel.org>,
        <zhengbin13@...wei.com>, <yi.zhang@...wei.com>,
        <chenxiang66@...ilicon.com>, <xiexiuqi@...wei.com>
Subject: Re: [PATCH] debugfs: fix potential infinite loop in
 debugfs_remove_recursive



On 2019/11/14 10:43, Steven Rostedt wrote:
> On Thu, 14 Nov 2019 10:01:23 +0800
> "yukuai (C)" <yukuai3@...wei.com> wrote:
> 
> 
>> Do you agree with that list_empty(&chile->d_subdirs) here is not
>> appropriate? Since it can't skip the subdirs that is not
>> simple_positive(simple_positive() will return false), which is the
>> reason of infinite loop.
> 
> I do agree that simple_empty() is wrong, for the reasons you pointed out.
> 
>>>> +		if (!simple_empty(child)) {
>>>
>>> Have you tried this with lockdep enabled? I'm thinking that you might
>>> get a splat with holding parent->d_lock and simple_empty(child) taking
>>> the child->d_lock.
>> The locks are taken and released in the right order:
>> take parent->d_lock
>> 	take child->d_lock
>> 		list_for_each_entry(c, &child->d_sundirs, d_child)
>> 			take c->d_lock
>> 			release c->d_lock
>> 	release child->d_lock
>> release parent->d_lock
>> I don't see anything wrong, am I missing something?
> 
> It should be fine, my worry is that we may be missing a lockdep
> annotation, that might confuse lockdep, as lockdep may see this as the
> same type of lock being taken, and wont know the order.
> 
> Have you tried this patch with lockdep enabled and tried to hit this
> code path?
I haven't tried yet. I'll try soon and show the result.
Thanks
Yu Kuai
> 
> -- Steve
> 
> .
>