Re: Lockdep false positive in sysfs

[Eric W. Biederman]

Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:

> On Thu, 26 Apr 2012, Tejun Heo wrote:
>
>> On Thu, Apr 26, 2012 at 02:14:30PM -0400, Alan Stern wrote:
>> > > > 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.
>> 
>> The depth is limited by USB spec.
>> 
>> > I agree; this doesn't seem like a good approach.
>> 
>> It sure isn't pretty but probably best matches the situation in the
>> sense that lockdep would actually be able to know about the nesting
>> going on.
>
> By the way, do you know why attribute structures allow for dynamic keys 
> as well as static keys?  I see dynamic keys are used by attribute 
> containers, but I don't understand why.

Some attributes are allocated dynamically, but they need static keys.
It is a lockdep requirement.  So for dynamically allocated attributes
we have to smack them so they have static keys.

Almost all sysfs attributes are declared statically so the optimization
of taking the key from the attribute structure saves modifying a lot of
code and makes the code that exists easy to get correct.

Dynamically allocated attributes break in a pretty obvious way when you
try to use them with lockdep enabled so I think we have a reasonable
solution.

>> > Another idea is to have A's method temporarily drop the sysfs readlock.  
>> > Of course that would put the onus on the USB core of guaranteeing that
>> > A cannot be removed while this happens, but we can handle that.
>> 
>> Yeah, that's an easier way out.  Please make it a proper sysfs API
>> call tho so that people working on sysfs later can know of the special
>> case.
>
> I will.
>
> Would it be better to release just the lockdep annotation while
> continuing to hold the actual lock, or to really drop the lock?

We can't really drop the ``lock''.  That would imply not waiting for
all of the methods using a sysfs attributes not to finish before we
remove a sysfs attribute.  Kaboom!

Probably what would be better would figure out how to sneak in something
that for this file we tell lockdep to ignore it like you do the device
locks.  As you do for the device layer locking.

I don't like the choices available at this junction.  I have seen some
nasty ABBA deadlocks with sysfs, and anything that makes those easier to
see.

One thing that looks promising after reading lwn yesterday is what Al
and Oleg are doing with fput and a per task work queue that runs before
we return to user space.  If you could use that we could have our
cake and eat it too.   You could schedule the work in a work queue but
you could also be guaranteed that there are not any locking problems.

Do you think you could investigate that possibility?

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/