Re: drm_vm.c:drm_mmap: possible circular locking dependency detected

[Tejun Heo <tj@kernel.org>]

Hello, Eric.

On 01/02/2010 12:16 AM, Eric W. Biederman wrote:
>>> kobject_del with a lock held scares me.
>>
>> I would not object at _all_ if sysfs fixed the locking here instead of in 
>> filldir.
> 
> I just sent you my sysfs filldir scalability patch, so we can take that
> red-herring off the plate.
> 
> The problem as I see it is that kobject_del is convenient.
> kobject_del waits until all of the sysfs show and store methods for
> that kobject have stopped executing.  Which imposes the rule that
> kobject_del can not be called with any locks held that are taken in a
> sysfs show or store method.  This is all invisible to lockdep as the
> wait is done with a completion and not a lock.

The synchronization against read/write ops is in sysfs_deactivate on
purpose so that drivers (most common users) don't have to worry about
sysfs ops accessing different parts of data structures once
device_del() is complete.  Implementing the exlusion at the driver
level is possible but not easy because some hardware devices are
represented with complex data structures, some of them are reused when
devices are exchanged and some sysfs ops end up accessing the
hardware.  So, it's often not possible to simply disassociate the data
structure and float it till the last reference goes away.  There needs
to be a synchronization point where the driver can tell that nothing
is accessing released data structure or hardware resource after it and
it's far easier to define it at the sysfs level.

> sysfs_deactivate happens in the device_del(), but if we were to move
> sysfs_deactivate into the final kobject_put then in theory we can
> continue to block and be friendly but not need to be called with
> locations where locks are held.

Nobody would know when that final put will actually happen.  In
progress sysfs ops might access the hardware after the hardware is
gone or replaced with another unit.

> Either way we will need some lockdep warnings for sysfs_deactivate
> so that the problem does not continue to hide and silently foul
> things up.  So I will see if I can cook something.

I don't think this is really relevant to the problem at hand but
adding lockdep annotations would definitely be beneficial, which BTW
is another reason to leave the synchronization in sysfs_deactivate as
the trade off is between deadlocks which can be detected somewhat
reliably with lockdep and scary race conditions which may involve
hardware in mysterious ways.

Thanks.

-- 
tejun