Re: Lockdep false positive in sysfs

[ebiederm@xmission.com (Eric W. Biederman)]

Tejun Heo <tj@kernel.org> writes:

> Hello, Alan.
>
> On Wed, Apr 25, 2012 at 02:58:28PM -0400, Alan Stern wrote:
>> Peter and Tejun:
>> 
>> Here's my problem, which affects several sysfs attribute methods in the 
>> USB subsystem:
>> 
>> Sysfs attribute A is attached to a USB device D.  When the user writes
>> to A, the corresponding store method unregisters D's children (it does
>> not unregister D, though).
>> 
>> Now, some of these children may also be USB devices (i.e., if D is a
>> hub), and therefore may have the same set of sysfs attributes.  As a
>> result, A's store method for D will end up removing the A attribute for
>> device E, where E is a child of D.

Before we get too far getting lockdep to shut up let me ask.  Is this
really safe?  The code looks like it tries to be safe but in the case
of recursion I don't buy it.  You should be having this same problem
with other locks and I don't see the use of mutex_lock_nested anywhere.

I looked into this a couple years ago with regards to pci devices, and
my conclusion at the time was that the device core nor the pci
code was multi-thread hotplug safe, and that the easiest fix was
to use a single threaded workqueue.

What I see right now with the device_lock and usb looks better but
I'm not at all certain I believe that if you attempt to remove different
levels of the hierarchy at the same time from different processes
that this code is really safe.  Could you explain to me how the
locking works in the usb layer to prevent problems with different
processes removing different layers of the same hierarchy?

Especially having it work without having having to do something
interesting with lockdep.

>> This causes lockdep to complain.  When A's method is called, sysfs
>> tells lockdep that it holds a readlock for the s_active "rwsem"
>> associated with the A attribute for D.  However the sysfs routine that
>> removes attributes tells lockdep that it is going to get a writelock
>> for the s_active associated with the A attribute for E, which is in the
>> same lockdep class since it belongs to the same attribute.
>
> Hmmm.... This happens because, by default, sysfs_dirents for the same
> attr share the same lockdep key.  This happens from
> sysfs_dirent_init_lockdep().  Hmm.... we can,
>
> * Somehow assign different keys to sysfs_dirents for the specific
>   attr.  Use array of attrs indexed by bus depth?

Possible with sysfs_attr_init but pretty ugly.  Especially since it
sounds like this is a situation that does not presuppose a maximum
depth.  I do remember that the lockdep keys must be statically allocated
which makes this a challenge.

Or it might be a remote possibility to adapt things a little bit.
What makes this different from most of the recursing lock strategies
that I know of is that we are actually in fact taking the same
"lock" in the same place, and performing the same action.

> * Add a flag / whatever to attr indicating that the files of the
>   attribute may be removed recursively (lockdep-wise) and update
>   either read or write path to use subclass.
>
> Any better ideas?

I'm not going to worry about it too much until someone explains to me
how usb hotplug is really safe and how of all the locks taken to
make it happen somehow only the sysfs lock triggers a lockdep warning
because we are recursing.  We should be seeing the same issue with
the device lock if the device lock actually provides sufficient
protection to be useful.

Eric