| Message ID | 49F6B1E6.7050008@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Apr 28, 2009 at 12:36:06AM -0700, Yinghai Lu wrote: > > while looking dev_set_name() calling, there is one > dev_set_name(&dev->dev, NULL) > to used to try to free the device name, before kfree that device. What's wrong with that? > need to move the check for device_add in > | commit 8a577ffc75d9194fe8cdb7479236f2081c26ca1f > | Author: Kay Sievers <kay.sievers@vrfy.org> > | Date: Sat Apr 18 15:05:45 2009 -0700 > | > | driver: dont update dev_name via device_add path > from kobject_set_name_vargs to kobject_add_vargs instead. > > in kobject_set_name_vargs will check if fmt is NULL. > > actually we need to use dev_set_name(,NULL) later on failing path > and release to prevent leaking Are you sure? confused, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH wrote: > On Tue, Apr 28, 2009 at 12:36:06AM -0700, Yinghai Lu wrote: >> while looking dev_set_name() calling, there is one >> dev_set_name(&dev->dev, NULL) >> to used to try to free the device name, before kfree that device. > > What's wrong with that? > >> need to move the check for device_add in >> | commit 8a577ffc75d9194fe8cdb7479236f2081c26ca1f >> | Author: Kay Sievers <kay.sievers@vrfy.org> >> | Date: Sat Apr 18 15:05:45 2009 -0700 >> | >> | driver: dont update dev_name via device_add path >> from kobject_set_name_vargs to kobject_add_vargs instead. >> >> in kobject_set_name_vargs will check if fmt is NULL. >> >> actually we need to use dev_set_name(,NULL) later on failing path >> and release to prevent leaking > > Are you sure? > > confused, > in arch/arm/common/sa111.c static int sa1111_init_one_child(struct sa1111 *sachip, struct resource *parent, struct sa1111_dev_info *info) { struct sa1111_dev *dev; int ret; dev = kzalloc(sizeof(struct sa1111_dev), GFP_KERNEL); if (!dev) { ret = -ENOMEM; goto out; } dev_set_name(&dev->dev, "%4.4lx", info->offset); dev->devid = info->devid; dev->dev.parent = sachip->dev; dev->dev.bus = &sa1111_bus_type; dev->dev.release = sa1111_dev_release; dev->dev.coherent_dma_mask = sachip->dev->coherent_dma_mask; dev->res.start = sachip->phys + info->offset; dev->res.end = dev->res.start + 511; dev->res.name = dev_name(&dev->dev); dev->res.flags = IORESOURCE_MEM; dev->mapbase = sachip->base + info->offset; dev->skpcr_mask = info->skpcr_mask; memmove(dev->irq, info->irq, sizeof(dev->irq)); ret = request_resource(parent, &dev->res); if (ret) { printk("SA1111: failed to allocate resource for %s\n", dev->res.name); dev_set_name(&dev->dev, NULL); kfree(dev); goto out; } when first dev_set_name is called, dev->dev.kobj.name will initialized from kmalloc. so before kfree(dev), do we need to kfree that name? YH -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 28, 2009 at 08:14:13AM -0700, Yinghai Lu wrote: > Greg KH wrote: > > On Tue, Apr 28, 2009 at 12:36:06AM -0700, Yinghai Lu wrote: > >> while looking dev_set_name() calling, there is one > >> dev_set_name(&dev->dev, NULL) > >> to used to try to free the device name, before kfree that device. > > > > What's wrong with that? > > > >> need to move the check for device_add in > >> | commit 8a577ffc75d9194fe8cdb7479236f2081c26ca1f > >> | Author: Kay Sievers <kay.sievers@vrfy.org> > >> | Date: Sat Apr 18 15:05:45 2009 -0700 > >> | > >> | driver: dont update dev_name via device_add path > >> from kobject_set_name_vargs to kobject_add_vargs instead. > >> > >> in kobject_set_name_vargs will check if fmt is NULL. > >> > >> actually we need to use dev_set_name(,NULL) later on failing path > >> and release to prevent leaking > > > > Are you sure? > > > > confused, > > > > in arch/arm/common/sa111.c > > > static int > sa1111_init_one_child(struct sa1111 *sachip, struct resource *parent, > struct sa1111_dev_info *info) > { > struct sa1111_dev *dev; > int ret; > > dev = kzalloc(sizeof(struct sa1111_dev), GFP_KERNEL); > if (!dev) { > ret = -ENOMEM; > goto out; > } > > dev_set_name(&dev->dev, "%4.4lx", info->offset); > dev->devid = info->devid; > dev->dev.parent = sachip->dev; > dev->dev.bus = &sa1111_bus_type; > dev->dev.release = sa1111_dev_release; > dev->dev.coherent_dma_mask = sachip->dev->coherent_dma_mask; > dev->res.start = sachip->phys + info->offset; > dev->res.end = dev->res.start + 511; > dev->res.name = dev_name(&dev->dev); > dev->res.flags = IORESOURCE_MEM; > dev->mapbase = sachip->base + info->offset; > dev->skpcr_mask = info->skpcr_mask; > memmove(dev->irq, info->irq, sizeof(dev->irq)); > > ret = request_resource(parent, &dev->res); > if (ret) { > printk("SA1111: failed to allocate resource for %s\n", > dev->res.name); > dev_set_name(&dev->dev, NULL); > kfree(dev); > goto out; > } > > > when first dev_set_name is called, dev->dev.kobj.name will initialized from kmalloc. > so before kfree(dev), do we need to kfree that name? Do a "put_device()" instead, and everything will be freed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6/lib/kobject.c =================================================================== --- linux-2.6.orig/lib/kobject.c +++ linux-2.6/lib/kobject.c @@ -218,8 +218,8 @@ int kobject_set_name_vargs(struct kobjec const char *old_name = kobj->name; char *s; - if (kobj->name && !fmt) - return 0; + if (!fmt) + goto out; kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs); if (!kobj->name) @@ -229,6 +229,7 @@ int kobject_set_name_vargs(struct kobjec while ((s = strchr(kobj->name, '/'))) s[0] = '!'; +out: kfree(old_name); return 0; } @@ -301,11 +302,16 @@ static int kobject_add_varg(struct kobje { int retval; + if (kobj->name && !fmt) + goto add_with_name; + retval = kobject_set_name_vargs(kobj, fmt, vargs); if (retval) { printk(KERN_ERR "kobject: can not set name properly!\n"); return retval; } + +add_with_name: kobj->parent = parent; return kobject_add_internal(kobj); }
while looking dev_set_name() calling, there is one dev_set_name(&dev->dev, NULL) to used to try to free the device name, before kfree that device. need to move the check for device_add in | commit 8a577ffc75d9194fe8cdb7479236f2081c26ca1f | Author: Kay Sievers <kay.sievers@vrfy.org> | Date: Sat Apr 18 15:05:45 2009 -0700 | | driver: dont update dev_name via device_add path from kobject_set_name_vargs to kobject_add_vargs instead. in kobject_set_name_vargs will check if fmt is NULL. actually we need to use dev_set_name(,NULL) later on failing path and release to prevent leaking [ Impact: make dev_set_name(, NULL) could kfree old name ] Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- lib/kobject.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html