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]

Message-ID: <20191113214307.29a8d001@oasis.local.home>
Date:   Wed, 13 Nov 2019 21:43:07 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     "yukuai (C)" <yukuai3@...wei.com>
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 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?

-- Steve