Skip to content

Commit

Permalink
Input: serio - fix potential deadlock when unbinding drivers
Browse files Browse the repository at this point in the history 
sysfs_remove_group() waits for sysfs attributes to be removed, therefore
we do not need to worry about driver-specific attributes being accessed
after driver has been detached from the device. In fact, attempts to take
serio->drv_mutex in attribute methods may lead to the following deadlock:

                                          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();

Fix this by removing calls to serio_[un]pin_driver() and functions themselves
and using driver-private mutexes to serialize access to attribute's set()
methods that may change device state.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
  • Loading branch information
Eric W. Biederman authored and Dmitry Torokhov committed Jan 6, 2010
1 parent abf2a11 commit 59b0151
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 82 deletions.
59 changes: 25 additions & 34 deletions drivers/input/keyboard/atkbd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,8 +225,10 @@ struct atkbd {

struct delayed_work event_work;
unsigned long event_jiffies;
struct mutex event_mutex;
unsigned long event_mask;

/* Serializes reconnect(), attr->set() and event work */
struct mutex mutex;
};

/*
Expand Down Expand Up @@ -577,7 +579,7 @@ static void atkbd_event_work(struct work_struct *work)
{
struct atkbd *atkbd = container_of(work, struct atkbd, event_work.work);

mutex_lock(&atkbd->event_mutex);
mutex_lock(&atkbd->mutex);

if (!atkbd->enabled) {
/*
Expand All @@ -596,7 +598,7 @@ static void atkbd_event_work(struct work_struct *work)
atkbd_set_repeat_rate(atkbd);
}

mutex_unlock(&atkbd->event_mutex);
mutex_unlock(&atkbd->mutex);
}

/*
Expand All @@ -612,7 +614,7 @@ static void atkbd_schedule_event_work(struct atkbd *atkbd, int event_bit)

atkbd->event_jiffies = jiffies;
set_bit(event_bit, &atkbd->event_mask);
wmb();
mb();
schedule_delayed_work(&atkbd->event_work, delay);
}

Expand Down Expand Up @@ -849,12 +851,13 @@ static void atkbd_disconnect(struct serio *serio)
{
struct atkbd *atkbd = serio_get_drvdata(serio);

sysfs_remove_group(&serio->dev.kobj, &atkbd_attribute_group);

atkbd_disable(atkbd);

/* make sure we don't have a command in flight */
cancel_delayed_work_sync(&atkbd->event_work);

sysfs_remove_group(&serio->dev.kobj, &atkbd_attribute_group);
input_unregister_device(atkbd->dev);
serio_close(serio);
serio_set_drvdata(serio, NULL);
Expand Down Expand Up @@ -1087,7 +1090,7 @@ static int atkbd_connect(struct serio *serio, struct serio_driver *drv)
atkbd->dev = dev;
ps2_init(&atkbd->ps2dev, serio);
INIT_DELAYED_WORK(&atkbd->event_work, atkbd_event_work);
mutex_init(&atkbd->event_mutex);
mutex_init(&atkbd->mutex);

switch (serio->id.type) {

Expand Down Expand Up @@ -1160,19 +1163,23 @@ static int atkbd_reconnect(struct serio *serio)
{
struct atkbd *atkbd = serio_get_drvdata(serio);
struct serio_driver *drv = serio->drv;
int retval = -1;

if (!atkbd || !drv) {
printk(KERN_DEBUG "atkbd: reconnect request, but serio is disconnected, ignoring...\n");
return -1;
}

mutex_lock(&atkbd->mutex);

atkbd_disable(atkbd);

if (atkbd->write) {
if (atkbd_probe(atkbd))
return -1;
goto out;

if (atkbd->set != atkbd_select_set(atkbd, atkbd->set, atkbd->extra))
return -1;
goto out;

atkbd_activate(atkbd);

Expand All @@ -1190,8 +1197,11 @@ static int atkbd_reconnect(struct serio *serio)
}

atkbd_enable(atkbd);
retval = 0;

return 0;
out:
mutex_unlock(&atkbd->mutex);
return retval;
}

static struct serio_device_id atkbd_serio_ids[] = {
Expand Down Expand Up @@ -1235,47 +1245,28 @@ 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);
struct atkbd *atkbd = serio_get_drvdata(serio);

out:
serio_unpin_driver(serio);
return retval;
return handler(atkbd, buf);
}

static ssize_t atkbd_attr_set_helper(struct device *dev, const char *buf, size_t count,
ssize_t (*handler)(struct atkbd *, const char *, size_t))
{
struct serio *serio = to_serio_port(dev);
struct atkbd *atkbd;
struct atkbd *atkbd = serio_get_drvdata(serio);
int retval;

retval = serio_pin_driver(serio);
retval = mutex_lock_interruptible(&atkbd->mutex);
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);
mutex_unlock(&atkbd->mutex);

return retval;
}

Expand Down
32 changes: 3 additions & 29 deletions drivers/input/mouse/psmouse-base.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1450,24 +1450,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,
Expand All @@ -1478,18 +1464,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);

Expand Down Expand Up @@ -1519,8 +1496,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;
}

Expand Down Expand Up @@ -1582,9 +1558,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) {
Expand Down
19 changes: 0 additions & 19 deletions include/linux/serio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

/*
Expand Down

0 comments on commit 59b0151

Please sign in to comment.