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

[Eric W. Biederman]

Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> writes:

> On Sun, Jan 03, 2010 at 02:57:15AM -0800, Eric W. Biederman wrote:
>> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> writes:
>> 
>> >
>> > Overall I am not concerned about lockdep bitching about serio because it
>> > still bitches if you simply reload psmouse on a box with Synaptics with a
>> > pass-through port even though there are nested annotations and it is
>> > silent first time around.
>> 
>> This is a new lockdep annotation, and looking into it this appears to
>> be a true possible deadlock in the serio/sysfs interactions.
>> 
>> We have serio_pin_driver() called from all of the sysfs attributes
>> which does:
>>    mutex_lock(&serio->drv_mutex);
>> 
>> We have serio_disconnect_driver() called on an unplug which does:
>>    mutex_lock(&serio->drv_mutex);
>> 
>> The deadlock potential is if someone reads say the psmouse rate
>> sysfs file while the mouse is being unplugged.  There is a race
>> such that we can have:
>> 
>> 						  sysfs_read_file()
>>                                                     fill_read_buffer()
>> 						       sysfs_get_active_two()
>> 							 psmouse_attr_show_helper()
>>                                         		   serio_pin_driver()
>> serio_disconnect_driver()		
>>   mutex_lock(&serio->drv_mutex);		
>> 				<----------------->	   mutex_lock(&serio_drv_mutex);
>>     psmouse_disconnect()
>>       sysfs_remove_group(... psmouse_attr_group);
>> 	....
>> 	sysfs_deactivate(); 
>> 	  wait_for_completion();
>> 
>> 
>> So it is unlikely but possible to deadlock by accessing a serio
>> attribute of a serio device that is being removed.
>
> Hmm, I guess I was too quick dismissing lockdep complaints here. Now
> that sysfs remove waits deadlock indeed is possible. Actually the locks
> on serio->drv_mutex in attributes were added to make sure we don't
> access device that was unbound from the driver through stale sysfs
> attributes.

Cool.  So we have solved the problem generically but we have left over
layer specific solutions.  That seems like a good problem to have.

>> What to do about it is another question.
>
> I think we should simply not take serio->drv_mutex in attributes and use
> driver-private mutex to serialize "set" methods that may alter device
> state.

Do you have any ideas what those might be?  It looks like we are only
talking about psmouse and atkbd.  So the audit for this chunk should
not be too bad.

The psmouse code already has a mutex on it's set operations only the
atkbd does not.  The atkbd code does do a driver stop/start, which is
similar (but race prone without the serio->drv_mutex).

Except for the lack of atkbd_enable/disable locking the patch below should
be good.  Opinions from someone who knows the serio code better than I do
would be helpful.

Eric


---

From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
Subject: [PATCH] serio:  Remove uneeded and deadlock prone serio_pin_driver

sysfs_remove_group waits for sysfs attributes to be removed
so we don't need to take a mutex in each of the attributes to
prevent remove while the code in the attribute is running.

This removes a theoretical deadlock possibility of a keyboard
or mouse hotplug and someone accessing a sysfs attribute.

Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
---
 drivers/input/keyboard/atkbd.c     |   27 +--------------------------
 drivers/input/mouse/psmouse-base.c |   32 +++-----------------------------
 include/linux/serio.h              |   19 -------------------
 3 files changed, 4 insertions(+), 74 deletions(-)

diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index a357357..7200100 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -1234,22 +1234,8 @@ static ssize_t atkbd_attr_show_helper(struct device *dev, char *buf,
 				ssize_t (*handler)(struct atkbd *, char *))
 {
 	struct serio *serio = to_serio_port(dev);
-	int retval;
-
-	retval = serio_pin_driver(serio);
-	if (retval)
-		return retval;
 
-	if (serio->drv != &atkbd_drv) {
-		retval = -ENODEV;
-		goto out;
-	}
-
-	retval = handler((struct atkbd *)serio_get_drvdata(serio), buf);
-
-out:
-	serio_unpin_driver(serio);
-	return retval;
+	return handler((struct atkbd *)serio_get_drvdata(serio), buf);
 }
 
 static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t count,
@@ -1259,22 +1245,11 @@ static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t
 	struct atkbd *atkbd;
 	int retval;
 
-	retval = serio_pin_driver(serio);
-	if (retval)
-		return retval;
-
-	if (serio->drv != &atkbd_drv) {
-		retval = -ENODEV;
-		goto out;
-	}
-
 	atkbd = serio_get_drvdata(serio);
 	atkbd_disable(atkbd);
 	retval = handler(atkbd, buf, count);
 	atkbd_enable(atkbd);
 
-out:
-	serio_unpin_driver(serio);
 	return retval;
 }
 
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index fd0bc09..59754d3 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -1447,24 +1447,10 @@ ssize_t psmouse_attr_show_helper(struct device *dev, struct device_attribute *de
 	struct serio *serio = to_serio_port(dev);
 	struct psmouse_attribute *attr = to_psmouse_attr(devattr);
 	struct psmouse *psmouse;
-	int retval;
-
-	retval = serio_pin_driver(serio);
-	if (retval)
-		return retval;
-
-	if (serio->drv != &psmouse_drv) {
-		retval = -ENODEV;
-		goto out;
-	}
 
 	psmouse = serio_get_drvdata(serio);
 
-	retval = attr->show(psmouse, attr->data, buf);
-
-out:
-	serio_unpin_driver(serio);
-	return retval;
+	return attr->show(psmouse, attr->data, buf);
 }
 
 ssize_t psmouse_attr_set_helper(struct device *dev, struct device_attribute *devattr,
@@ -1475,18 +1461,9 @@ ssize_t psmouse_attr_set_helper(struct device *dev, struct device_attribute *dev
 	struct psmouse *psmouse, *parent = NULL;
 	int retval;
 
-	retval = serio_pin_driver(serio);
-	if (retval)
-		return retval;
-
-	if (serio->drv != &psmouse_drv) {
-		retval = -ENODEV;
-		goto out_unpin;
-	}
-
 	retval = mutex_lock_interruptible(&psmouse_mutex);
 	if (retval)
-		goto out_unpin;
+		goto out;
 
 	psmouse = serio_get_drvdata(serio);
 
@@ -1516,8 +1493,7 @@ ssize_t psmouse_attr_set_helper(struct device *dev, struct device_attribute *dev
 
  out_unlock:
 	mutex_unlock(&psmouse_mutex);
- out_unpin:
-	serio_unpin_driver(serio);
+ out:
 	return retval;
 }
 
@@ -1579,9 +1555,7 @@ static ssize_t psmouse_attr_set_protocol(struct psmouse *psmouse, void *data, co
 		}
 
 		mutex_unlock(&psmouse_mutex);
-		serio_unpin_driver(serio);
 		serio_unregister_child_port(serio);
-		serio_pin_driver_uninterruptible(serio);
 		mutex_lock(&psmouse_mutex);
 
 		if (serio->drv != &psmouse_drv) {
diff --git a/include/linux/serio.h b/include/linux/serio.h
index e2f3044..813d26c 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -136,25 +136,6 @@ static inline void serio_continue_rx(struct serio *serio)
 	spin_unlock_irq(&serio->lock);
 }
 
-/*
- * Use the following functions to pin serio's driver in process context
- */
-static inline int serio_pin_driver(struct serio *serio)
-{
-	return mutex_lock_interruptible(&serio->drv_mutex);
-}
-
-static inline void serio_pin_driver_uninterruptible(struct serio *serio)
-{
-	mutex_lock(&serio->drv_mutex);
-}
-
-static inline void serio_unpin_driver(struct serio *serio)
-{
-	mutex_unlock(&serio->drv_mutex);
-}
-
-
 #endif
 
 /*
-- 
1.6.5.2.143.g8cc62

--
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/