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

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

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On 01/03/2010 06:49 AM, Eric W. Biederman wrote:
>>>> 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.
>> 
>> Alright than that is a bad possible split of the functionality.  Which
>> is all I was suggesting splitting the functionality not doing away
>> with the wait or moving it to a point where the wait would not work.
>> It was simply my bad assumption that the final kobject_put would
>> happen before the module that controlled that kobject could be
>> removed.
>
> The module should stay around.  The severing is necessary to protect
> driver internal data structures and possibly removed or reattached (to
> a different driver) hardware.

Removed driver hardware isn't something sysfs can really guard
against, although it can help to make the window of vulnerability
smaller.  Protecting driver internal data structures if we can does
seem reasonable.

The case I was thinking of in particular is when someone does:
"rmmod driver" I think device_del protects from the code going away
today.

>> I still think it might make sense to separate kobject_del into two
>> parts.  One that we call with the locks held and one without, but that
>> does seem to be applicable to only a very small set of cases and our
>> problems appear to be much larger than that.
>
> If such separation is necessary, we can implement the split interface
> while leaving kobject_del() as is feature-wise and convert the
> offending ones to use the split interface but I think it would be
> better to simply fix the offending ones if there aren't too many and
> they're easily fixable.  Let's see how many lockdep warnings turn up.

- We have the network stack.
  I have hacked around that (when I thought it was a singleton)
  by introducing the idiom:

	if (!rtnl_trylock())
		return restart_sysscall();

  But that isn't sustainable, as there is already one new entry that
  just does rntl_lock unconditionally.

  Maybe we can move the device_del out from under the rtnl_lock, but I
  have my doubts.  Certainly the proc and sysctl bits (which have the
  same issue look more difficult.

- We almost have an issue in ext4.
  Device_del is certainly called under lock_kernel() and lock_super().

- We have what a cpu_hotplug.lock issue with
  /sys/devices/system/cpu/cpuN/microcode/reload, a variant of the problem
  that triggered this discussion and it looks very non-trivial to solve.

So I'm not certain what to say except that we have longstanding problems.

Eric