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

[Steven Rostedt]

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?

-- Steve