Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_ioctl.c | 73 ++++++++++++++++++++++++++++++++---------- 1 files changed, 55 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 9b9ff46..197267b 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -94,17 +94,24 @@ int drm_setunique(struct drm_device *dev, void *data, master->unique_len = u->unique_len; master->unique_size = u->unique_len + 1; master->unique = kmalloc(master->unique_size, GFP_KERNEL); - if (!master->unique) - return -ENOMEM; - if (copy_from_user(master->unique, u->unique, master->unique_len)) - return -EFAULT; + if (!master->unique) { + ret = -ENOMEM; + goto err; + } + + if (copy_from_user(master->unique, u->unique, master->unique_len)) { + ret = -EFAULT; + goto err; + }
master->unique[master->unique_len] = '\0';
dev->devname = kmalloc(strlen(dev->driver->pci_driver.name) + strlen(master->unique) + 2, GFP_KERNEL); - if (!dev->devname) - return -ENOMEM; + if (!dev->devname) { + ret = -ENOMEM; + goto err; + }
sprintf(dev->devname, "%s@%s", dev->driver->pci_driver.name, master->unique); @@ -113,24 +120,39 @@ int drm_setunique(struct drm_device *dev, void *data, * busid. */ ret = sscanf(master->unique, "PCI:%d:%d:%d", &bus, &slot, &func); - if (ret != 3) - return -EINVAL; + if (ret != 3) { + ret = -EINVAL; + goto err; + } + domain = bus >> 8; bus &= 0xff;
if ((domain != drm_get_pci_domain(dev)) || (bus != dev->pdev->bus->number) || (slot != PCI_SLOT(dev->pdev->devfn)) || - (func != PCI_FUNC(dev->pdev->devfn))) - return -EINVAL; + (func != PCI_FUNC(dev->pdev->devfn))) { + ret = -EINVAL; + goto err; + }
return 0; + +err: + kfree(dev->devname); + dev->devname = NULL; + + kfree(master->unique); + master->unique = NULL; + master->unique_len = 0; + master->unique_size = 0; + return ret; }
static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) { struct drm_master *master = file_priv->master; - int len; + int len, ret;
if (master->unique != NULL) return -EBUSY; @@ -138,28 +160,41 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) master->unique_len = 40; master->unique_size = master->unique_len; master->unique = kmalloc(master->unique_size, GFP_KERNEL); - if (master->unique == NULL) - return -ENOMEM; + if (master->unique == NULL) { + ret = -ENOMEM; + goto err; + }
len = snprintf(master->unique, master->unique_len, "pci:%04x:%02x:%02x.%d", drm_get_pci_domain(dev), dev->pdev->bus->number, PCI_SLOT(dev->pdev->devfn), PCI_FUNC(dev->pdev->devfn)); - if (len >= master->unique_len) + if (len >= master->unique_len) { DRM_ERROR("buffer overflow"); - else + ret = -EINVAL; + goto err; + } else master->unique_len = len;
dev->devname = kmalloc(strlen(dev->driver->pci_driver.name) + master->unique_len + 2, GFP_KERNEL); - if (dev->devname == NULL) - return -ENOMEM; + if (dev->devname == NULL) { + ret = -ENOMEM; + goto err; + }
sprintf(dev->devname, "%s@%s", dev->driver->pci_driver.name, master->unique);
return 0; + +err: + kfree(master->unique); + master->unique = NULL; + master->unique_len = 0; + master->unique_size = 0; + return ret; }
/** @@ -323,7 +358,9 @@ int drm_setversion(struct drm_device *dev, void *data, struct drm_file *file_pri /* * Version 1.1 includes tying of DRM to specific device */ - drm_set_busid(dev, file_priv); + retcode = drm_set_busid(dev, file_priv); + if (retcode) + goto done; } }
The device name is tightly coupled and created at the same time as the master->unique address, so we need to free it with the master. Currently we overwrite it each time we create a new master:
unreferenced object 0xe32c54b0 (size 32): comm "Xorg", pid 1455, jiffies 4294721798 (age 3196.879s) hex dump (first 32 bytes): 69 39 31 35 40 70 63 69 3a 30 30 30 30 3a 30 30 i915@pci:0000:00 3a 30 32 2e 30 00 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 :02.0.kkkkkkkkk. backtrace: [<c04e5657>] create_object+0x124/0x1f1 [<c07cf0f0>] kmemleak_alloc+0x4c/0x90 [<c04db84c>] __kmalloc+0x155/0x175 [<f8316665>] drm_setversion+0x11d/0x1b1 [drm] [<f83148d4>] drm_ioctl+0x29a/0x356 [drm] [<c04f27c4>] vfs_ioctl+0x33/0x91 [<c04f31cf>] do_vfs_ioctl+0x46b/0x496 [<c04f3240>] sys_ioctl+0x46/0x66 [<c040325f>] sysenter_do_call+0x12/0x38 [<ffffffff>] 0xffffffff
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/drm_stub.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index a0c365f..4aa84f4 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -156,6 +156,9 @@ static void drm_master_destroy(struct kref *kref) master->unique_len = 0; }
+ kfree(dev->devname); + dev->devname = NULL; + list_for_each_entry_safe(pt, next, &master->magicfree, head) { list_del(&pt->head); drm_ht_remove_item(&master->magiclist, &pt->hash_item);
A side-effect of being able to use custom page allocations with the sg_table is that it cannot reap the partially constructed scatterlist if fails to allocate a page. So we need to call sg_free_table() ourselves if sg_alloc_table() fails.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc Dave Airlie airlied@redhat.com --- drivers/char/agp/intel-gtt.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index f97122a..5615d70 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -104,7 +104,7 @@ static int intel_agp_map_memory(struct agp_memory *mem) DBG("try mapping %lu pages\n", (unsigned long)mem->page_count);
if (sg_alloc_table(&st, mem->page_count, GFP_KERNEL)) - return -ENOMEM; + goto err;
mem->sg_list = sg = st.sgl;
@@ -113,11 +113,14 @@ static int intel_agp_map_memory(struct agp_memory *mem)
mem->num_sg = pci_map_sg(intel_private.pcidev, mem->sg_list, mem->page_count, PCI_DMA_BIDIRECTIONAL); - if (unlikely(!mem->num_sg)) { - intel_agp_free_sglist(mem); - return -ENOMEM; - } + if (unlikely(!mem->num_sg)) + goto err; + return 0; + +err: + sg_free_table(&st); + return -ENOMEM; }
static void intel_agp_unmap_memory(struct agp_memory *mem)
On 2010.07.24 18:29:37 +0100, Chris Wilson wrote:
A side-effect of being able to use custom page allocations with the sg_table is that it cannot reap the partially constructed scatterlist if fails to allocate a page. So we need to call sg_free_table() ourselves if sg_alloc_table() fails.
Why? Doesn't sg_alloc_table() handle the failure case to call sg_free_table() already?
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc Dave Airlie airlied@redhat.com
drivers/char/agp/intel-gtt.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index f97122a..5615d70 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -104,7 +104,7 @@ static int intel_agp_map_memory(struct agp_memory *mem) DBG("try mapping %lu pages\n", (unsigned long)mem->page_count);
if (sg_alloc_table(&st, mem->page_count, GFP_KERNEL))
return -ENOMEM;
goto err;mem->sg_list = sg = st.sgl;
@@ -113,11 +113,14 @@ static int intel_agp_map_memory(struct agp_memory *mem)
mem->num_sg = pci_map_sg(intel_private.pcidev, mem->sg_list, mem->page_count, PCI_DMA_BIDIRECTIONAL);
- if (unlikely(!mem->num_sg)) {
intel_agp_free_sglist(mem);return -ENOMEM;- }
- if (unlikely(!mem->num_sg))
goto err;- return 0;
+err:
- sg_free_table(&st);
- return -ENOMEM;
}
static void intel_agp_unmap_memory(struct agp_memory *mem)
1.7.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 27 Jul 2010 10:00:07 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
On 2010.07.24 18:29:37 +0100, Chris Wilson wrote:
A side-effect of being able to use custom page allocations with the sg_table is that it cannot reap the partially constructed scatterlist if fails to allocate a page. So we need to call sg_free_table() ourselves if sg_alloc_table() fails.
Why? Doesn't sg_alloc_table() handle the failure case to call sg_free_table() already?
sg_alloc_table() is just a wrapper around __sg_alloc_table() that passes in the default alloc_func. So for __sg_alloc_table() to do the cleanup we would need to also pass in the free_func. Since it doesn't, it can't free the partially constructed table. So to fix this we have the choice of changing the interface (and add extra overhead to the common path) or trivially rearrange our failure path.
On Sat, 24 Jul 2010 18:29:37 +0100, Chris Wilson chris@chris-wilson.co.uk wrote:
A side-effect of being able to use custom page allocations with the sg_table is that it cannot reap the partially constructed scatterlist if fails to allocate a page. So we need to call sg_free_table() ourselves if sg_alloc_table() fails.
Applied to -next.
On Sat, 24 Jul 2010 18:29:35 +0100, Chris Wilson chris@chris-wilson.co.uk wrote:
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk @@ -323,7 +358,9 @@ int drm_setversion(struct drm_device *dev, void *data, struct drm_file *file_pri /* * Version 1.1 includes tying of DRM to specific device */
drm_set_busid(dev, file_priv);
retcode = drm_set_busid(dev, file_priv);if (retcode)goto done;
And actually returning the error code is breaking xorg 1.8.99... Hmm.
v2: Userspace (notably xf86-video-{intel,ati}) became confused when drmSetInterfaceVersion() started returning -EBUSY as they used a second call (the first done in drmOpen()) to check their master credentials. Since userspace wants to be able to repeatedly call drmSetInterfaceVersion() allow them to do so.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_ioctl.c | 79 ++++++++++++++++++++++++++++++++---------- 1 files changed, 60 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 9b9ff46..f107fff 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -64,6 +64,19 @@ int drm_getunique(struct drm_device *dev, void *data, return 0; }
+static void +drm_unset_busid(struct drm_device *dev, + struct drm_master *master) +{ + kfree(dev->devname); + dev->devname = NULL; + + kfree(master->unique); + master->unique = NULL; + master->unique_len = 0; + master->unique_size = 0; +} + /** * Set the bus id. * @@ -94,17 +107,24 @@ int drm_setunique(struct drm_device *dev, void *data, master->unique_len = u->unique_len; master->unique_size = u->unique_len + 1; master->unique = kmalloc(master->unique_size, GFP_KERNEL); - if (!master->unique) - return -ENOMEM; - if (copy_from_user(master->unique, u->unique, master->unique_len)) - return -EFAULT; + if (!master->unique) { + ret = -ENOMEM; + goto err; + } + + if (copy_from_user(master->unique, u->unique, master->unique_len)) { + ret = -EFAULT; + goto err; + }
master->unique[master->unique_len] = '\0';
dev->devname = kmalloc(strlen(dev->driver->pci_driver.name) + strlen(master->unique) + 2, GFP_KERNEL); - if (!dev->devname) - return -ENOMEM; + if (!dev->devname) { + ret = -ENOMEM; + goto err; + }
sprintf(dev->devname, "%s@%s", dev->driver->pci_driver.name, master->unique); @@ -113,53 +133,72 @@ int drm_setunique(struct drm_device *dev, void *data, * busid. */ ret = sscanf(master->unique, "PCI:%d:%d:%d", &bus, &slot, &func); - if (ret != 3) - return -EINVAL; + if (ret != 3) { + ret = -EINVAL; + goto err; + } + domain = bus >> 8; bus &= 0xff;
if ((domain != drm_get_pci_domain(dev)) || (bus != dev->pdev->bus->number) || (slot != PCI_SLOT(dev->pdev->devfn)) || - (func != PCI_FUNC(dev->pdev->devfn))) - return -EINVAL; + (func != PCI_FUNC(dev->pdev->devfn))) { + ret = -EINVAL; + goto err; + }
return 0; + +err: + drm_unset_busid(dev, master); + return ret; }
static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv) { struct drm_master *master = file_priv->master; - int len; + int len, ret;
if (master->unique != NULL) - return -EBUSY; + drm_unset_busid(dev, master);
master->unique_len = 40; master->unique_size = master->unique_len; master->unique = kmalloc(master->unique_size, GFP_KERNEL); - if (master->unique == NULL) - return -ENOMEM; + if (master->unique == NULL) { + ret = -ENOMEM; + goto err; + }
len = snprintf(master->unique, master->unique_len, "pci:%04x:%02x:%02x.%d", drm_get_pci_domain(dev), dev->pdev->bus->number, PCI_SLOT(dev->pdev->devfn), PCI_FUNC(dev->pdev->devfn)); - if (len >= master->unique_len) + if (len >= master->unique_len) { DRM_ERROR("buffer overflow"); - else + ret = -EINVAL; + goto err; + } else master->unique_len = len;
dev->devname = kmalloc(strlen(dev->driver->pci_driver.name) + master->unique_len + 2, GFP_KERNEL); - if (dev->devname == NULL) - return -ENOMEM; + if (dev->devname == NULL) { + ret = -ENOMEM; + goto err; + }
sprintf(dev->devname, "%s@%s", dev->driver->pci_driver.name, master->unique);
return 0; + +err: + drm_unset_busid(dev, master); + return ret; }
/** @@ -323,7 +362,9 @@ int drm_setversion(struct drm_device *dev, void *data, struct drm_file *file_pri /* * Version 1.1 includes tying of DRM to specific device */ - drm_set_busid(dev, file_priv); + retcode = drm_set_busid(dev, file_priv); + if (retcode) + goto done; } }
On Sat, 2010-07-24 at 19:34 +0100, Chris Wilson wrote:
v2: Userspace (notably xf86-video-{intel,ati}) became confused when drmSetInterfaceVersion() started returning -EBUSY as they used a second call (the first done in drmOpen()) to check their master credentials. Since userspace wants to be able to repeatedly call drmSetInterfaceVersion() allow them to do so.
care to rebase on drm-core-next?
this fails to apply since the drm platform changes.
Dave.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Dave Airlie -
Eric Anholt -
Zhenyu Wang