Re: [PATCH] debugfs: fix potential infinite loop in debugfs_remove_recursive

From: yukuai (C)
Date: Wed Nov 13 2019 - 22:20:55 EST




On 2019/11/14 10:43, Steven Rostedt wrote:
On Thu, 14 Nov 2019 10:01:23 +0800
"yukuai (C)" <yukuai3@xxxxxxxxxx> 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

.