Re: [PATCH v2 06/22] vga_switcheroo: Lock/unlock DDC lines harder

On Fri, Mar 27, 2015 at 12:29:40PM +0100, Lukas Wunner wrote:

Unlock DDC lines if drm_probe_ddc() fails.

If the inactive client registers before the active client then old_ddc_owner cannot be determined with find_active_client() (null pointer dereference). Therefore change semantics of the ->switch_ddc handler callback to return old_ddc_owner on success or a negative int on failure.

Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to avoid locking inversion when one client locks vgasr_mutex on entering vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another client is holding ddc_lock and tries to acquire vgasr_mutex on entering vga_switcheroo_unlock_ddc().

Lock ddc_lock in stage2 to avoid race condition where reading the EDID and switching happens simultaneously.

Switch DDC lines on MIGD / MDIS commands and on runtime suspend.

So essentially you have

bool locked = mutex_trylock();

/* nothing that looks at locked at all */

if (locked) mutex_unlock;

This doesn't protect anything at all and makes it look _very_ fishy.

I haven't dug deeper yet. -Daniel

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Paul Hordiienko pvt.gord@gmail.com [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] Tested-by: William Brown william@blackhats.net.au [MBP 8,2 2011 intel SNB + amd turks pre-retina] Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] Tested-by: Bruno Bierbaumer bruno@bierbaumer.net [MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]

Signed-off-by: Lukas Wunner lukas@wunner.de

drivers/gpu/drm/drm_edid.c | 9 ++-- drivers/gpu/vga/vga_switcheroo.c | 110 ++++++++++++++++++++++---------------- drivers/platform/x86/apple-gmux.c | 15 +++++- include/linux/vga_switcheroo.h | 3 +- 4 files changed, 87 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 505eed1..cdb2fa1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { struct edid *edid;

• struct pci_dev *pdev = connector->dev->pdev;

• vga_switcheroo_lock_ddc(connector->dev->pdev);

• vga_switcheroo_lock_ddc(pdev);

• if (!drm_probe_ddc(adapter))

• if (!drm_probe_ddc(adapter)) {

• vga_switcheroo_unlock_ddc(pdev);
  

  return NULL;

• }

  edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter); if (edid) drm_get_displayid(connector, edid);

• vga_switcheroo_unlock_ddc(connector->dev->pdev);

• vga_switcheroo_unlock_ddc(pdev);

  return edid;

} diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 0223020..f02e7fc 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -9,12 +9,13 @@

Switcher interface - methods require for ATPX and DCM

• switchto - this throws the output MUX switch
• • discrete_set_power - sets the power state for the discrete card

• • switch_ddc - switch only DDC lines, return old DDC owner (or < 0 on failure)
• • power_state - sets the power state for either GPU

  GPU driver interface

  • set_gpu_state - this should do the equiv of s/r for the card
    • this should *not* set the discrete power state

• • switch_check - check if the device is in a position to switch now

• • can_switch - check if the device is in a position to switch now
  */

#include <linux/module.h> @@ -59,7 +60,7 @@ struct vgasr_priv { struct vga_switcheroo_handler *handler;

struct mutex ddc_lock;

• struct pci_dev *old_ddc_owner;

• enum vga_switcheroo_client_id old_ddc_owner;

};

#define ID_BIT_AUDIO 0x100 @@ -276,33 +277,24 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);

int vga_switcheroo_lock_ddc(struct pci_dev *pdev) {

• struct vga_switcheroo_client *client;
• int ret = 0;
• int id;

• int ret, id;

  mutex_lock(&vgasr_mutex);

• if (!vgasr_priv.handler) {

• if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { ret = -ENODEV; goto out; }

• if (vgasr_priv.handler->switch_ddc) {

• mutex_lock(&vgasr_priv.ddc_lock);
  

• mutex_lock(&vgasr_priv.ddc_lock);
• id = vgasr_priv.handler->get_client_id(pdev);
• ret = vgasr_priv.handler->switch_ddc(id);

• client = find_active_client(&vgasr_priv.clients);
  

• if (!client) {
  

• 	mutex_unlock(&vgasr_priv.ddc_lock);
  

• 	ret = -ENODEV;
  

• 	goto out;
  

• }
  

• vgasr_priv.old_ddc_owner = client->pdev;
  

• if (client->pdev == pdev)
  

• 	goto out;
  

• id = vgasr_priv.handler->get_client_id(pdev);
  

• ret = vgasr_priv.handler->switch_ddc(id);
  

• }

• if (ret < 0) {

• pr_err("vga_switcheroo: failed to switch DDC lines\n");
  

• mutex_unlock(&vgasr_priv.ddc_lock);
  

• } else

• vgasr_priv.old_ddc_owner = ret;
  
  

out: mutex_unlock(&vgasr_mutex); @@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc);

int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) {

• int ret = 0;
• int id;
• mutex_lock(&vgasr_mutex);

• int ret, id;
• bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex);

• if (!vgasr_priv.handler) {

• ret = -ENODEV;
  

• if (!mutex_is_locked(&vgasr_priv.ddc_lock)) {

• ret = -EINVAL;
  

  goto out; }

• if (vgasr_priv.handler->switch_ddc) {

• if (vgasr_priv.old_ddc_owner != pdev) {
  

• 	id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner);
  

• 	ret = vgasr_priv.handler->switch_ddc(id);
  

• }
  

• vgasr_priv.old_ddc_owner = NULL;
  

• if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {

• pr_err("vga_switcheroo: handler disappeared\n");
  

  mutex_unlock(&vgasr_priv.ddc_lock);

• ret = -ENODEV;
  

• goto out;
  

  }
• id = vgasr_priv.handler->get_client_id(pdev);
• if (vgasr_priv.old_ddc_owner != id) {

• ret = vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner);
  

• if (ret < 0)
  

• 	pr_err("vga_switcheroo: failed to switch DDC lines\n");
  

• } else

• ret = vgasr_priv.old_ddc_owner;
  

• mutex_unlock(&vgasr_priv.ddc_lock);

out:

• mutex_unlock(&vgasr_mutex);

• if (vgasr_mutex_acquired)

• mutex_unlock(&vgasr_mutex);
  

  return ret;

} EXPORT_SYMBOL(vga_switcheroo_unlock_ddc); @@ -433,14 +433,26 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) }

if (vgasr_priv.handler->switch_ddc) {

• mutex_lock(&vgasr_priv.ddc_lock);
  

  ret = vgasr_priv.handler->switch_ddc(new_client->id);

• if (ret)
  

• mutex_unlock(&vgasr_priv.ddc_lock);
  

• if (ret < 0) {
  

• 	pr_err("vga_switcheroo: failed to switch DDC lines\n");
  return ret;
  

• }
  

  }

  ret = vgasr_priv.handler->switchto(new_client->id);

• if (ret)

• goto restore_ddc;
  

• if (ret) {

• pr_err("vga_switcheroo: failed to switch to client %d\n",
  

•        new_client->id);
  

• active->active = true;
  

• /* restore DDC lines */
  

• mutex_lock(&vgasr_priv.ddc_lock);
  

• vgasr_priv.handler->switch_ddc(active->id);
  

• mutex_unlock(&vgasr_priv.ddc_lock);
  

• return ret;
  

• }

  if (new_client->ops->reprobe) new_client->ops->reprobe(new_client->pdev);

@@ -452,14 +464,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)

new_client->active = true; return 0;

-restore_ddc:

• if (vgasr_priv.handler->switch_ddc) {

• int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
  

• 		VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
  

• ret = vgasr_priv.handler->switch_ddc(id);
  

• }
• return ret;

}

static bool check_can_switch(void) @@ -561,6 +565,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, vgasr_priv.delayed_switch_active = false;

if (just_mux) {

• if (vgasr_priv.handler->switch_ddc) {
  

• 	mutex_lock(&vgasr_priv.ddc_lock);
  

• 	ret = vgasr_priv.handler->switch_ddc(client_id);
  

• 	mutex_unlock(&vgasr_priv.ddc_lock);
  

• 	if (ret < 0) {
  

• 		pr_err("vga_switcheroo: failed to switch DDC lines\n");
  

• 		goto out;
  

• 	}
  

• }
  

  ret = vgasr_priv.handler->switchto(client_id); goto out; }

@@ -716,6 +729,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) ret = dev->bus->pm->runtime_suspend(dev); if (ret) return ret;

• if (vgasr_priv.handler->switch_ddc) {

• mutex_lock(&vgasr_priv.ddc_lock);
  

• ret = vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD);
  

• mutex_unlock(&vgasr_priv.ddc_lock);
  

• if (ret < 0)
  

• 	pr_err("vga_switcheroo: failed to switch DDC lines\n");
  

• } if (vgasr_priv.handler->switchto) vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index f0a55a4..08bdf1e 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = {

static int gmux_switch_ddc(enum vga_switcheroo_client_id id) {

• enum vga_switcheroo_client_id old_ddc_owner;
• if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)

• old_ddc_owner = VGA_SWITCHEROO_IGD;
  

• else

• old_ddc_owner = VGA_SWITCHEROO_DIS;
  

• pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
• if (id == old_ddc_owner)

• return old_ddc_owner;
  

• if (id == VGA_SWITCHEROO_IGD) gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1); else gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);

• return 0;

• return old_ddc_owner;

}

static int gmux_switchto(enum vga_switcheroo_client_id id) diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index 352bef3..39b25b0 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {} static inline int vga_switcheroo_register_client(struct pci_dev *dev, const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; } static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {} -static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; } +static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; } +static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; } static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops, -- 1.8.5.2 (Apple Git-48)

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel