| Message ID | 1496866004-32328-1-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 07, 2017 at 03:06:44PM -0500, Mike Christie wrote: > It looks like there might be 2 issues with the uio_device allocation, or it > looks like we are leaking the device for possibly a specific type of device > case that I could not find but one of you may know about. > > Issues: > 1. We use devm_kzalloc to allocate the uio_device, but the release > function, devm_kmalloc_release, is just a noop, so the memory is never freed. What do you mean by this? If the release function is a noop, lots of memory in the kernel is leaking. UIO shouldn't have to do anything special here, is the devm api somehow broken? If so, let's fix that, not paper over all other driver bugs by moving the UIO code away from it :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/2017 09:01 AM, Greg KH wrote: > On Wed, Jun 07, 2017 at 03:06:44PM -0500, Mike Christie wrote: >> It looks like there might be 2 issues with the uio_device allocation, or it >> looks like we are leaking the device for possibly a specific type of device >> case that I could not find but one of you may know about. >> >> Issues: >> 1. We use devm_kzalloc to allocate the uio_device, but the release >> function, devm_kmalloc_release, is just a noop, so the memory is never freed. > > What do you mean by this? If the release function is a noop, lots of > memory in the kernel is leaking. UIO shouldn't have to do anything > special here, is the devm api somehow broken? Sorry. I misdiagnosed the problem. It's a noop, but we did kfree on the entire devres and its data later. The problem I was hitting is that memory is not freed until the parent is removed. __uio_register_device does: idev = devm_kzalloc(parent, sizeof(*idev), GFP_KERNEL); if (!idev) { return -ENOMEM; } so the devres's memory is associated with the parent. Is that intentional? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/2017 07:16 PM, Mike Christie wrote: > On 06/13/2017 09:01 AM, Greg KH wrote: >> > On Wed, Jun 07, 2017 at 03:06:44PM -0500, Mike Christie wrote: >>> >> It looks like there might be 2 issues with the uio_device allocation, or it >>> >> looks like we are leaking the device for possibly a specific type of device >>> >> case that I could not find but one of you may know about. >>> >> >>> >> Issues: >>> >> 1. We use devm_kzalloc to allocate the uio_device, but the release >>> >> function, devm_kmalloc_release, is just a noop, so the memory is never freed. >> > >> > What do you mean by this? If the release function is a noop, lots of >> > memory in the kernel is leaking. UIO shouldn't have to do anything >> > special here, is the devm api somehow broken? > Sorry. I misdiagnosed the problem. It's a noop, but we did kfree on the > entire devres and its data later. > > The problem I was hitting is that memory is not freed until the parent > is removed. __uio_register_device does: > > idev = devm_kzalloc(parent, sizeof(*idev), GFP_KERNEL); > if (!idev) { > return -ENOMEM; > } > > so the devres's memory is associated with the parent. Is that intentional? > What I meant is that it I can send a patch to just fix up the devm_kzalloc use in uio.c, so it gets the device struct for the uio device instead of the parent. However, it looks like the existing code using the parent prevents a crash. If the child is hot unplugged/removed, and uio_unregister_device ends up freeing the idev, then later when the userspace application does a close on the uio device we would try to access the freed idev in uio_release. If the devm_kzalloc parent use was meant for that hot unplug case, then I can also look into how to fix the drivers too. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 13, 2017 at 07:35:51PM -0500, Mike Christie wrote: > On 06/13/2017 07:16 PM, Mike Christie wrote: > > On 06/13/2017 09:01 AM, Greg KH wrote: > >> > On Wed, Jun 07, 2017 at 03:06:44PM -0500, Mike Christie wrote: > >>> >> It looks like there might be 2 issues with the uio_device allocation, or it > >>> >> looks like we are leaking the device for possibly a specific type of device > >>> >> case that I could not find but one of you may know about. > >>> >> > >>> >> Issues: > >>> >> 1. We use devm_kzalloc to allocate the uio_device, but the release > >>> >> function, devm_kmalloc_release, is just a noop, so the memory is never freed. > >> > > >> > What do you mean by this? If the release function is a noop, lots of > >> > memory in the kernel is leaking. UIO shouldn't have to do anything > >> > special here, is the devm api somehow broken? > > Sorry. I misdiagnosed the problem. It's a noop, but we did kfree on the > > entire devres and its data later. > > > > The problem I was hitting is that memory is not freed until the parent > > is removed. __uio_register_device does: > > > > idev = devm_kzalloc(parent, sizeof(*idev), GFP_KERNEL); > > if (!idev) { > > return -ENOMEM; > > } > > > > so the devres's memory is associated with the parent. Is that intentional? > > > > What I meant is that it I can send a patch to just fix up the > devm_kzalloc use in uio.c, so it gets the device struct for the uio > device instead of the parent. > > However, it looks like the existing code using the parent prevents a > crash. If the child is hot unplugged/removed, and uio_unregister_device > ends up freeing the idev, then later when the userspace application does > a close on the uio device we would try to access the freed idev in > uio_release. > > If the devm_kzalloc parent use was meant for that hot unplug case, then > I can also look into how to fix the drivers too. Yeah, I don't know why it is tied to the parent, I'll take a patch to fix that and let's see what breaks :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index beb5f09..00fde52 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -1299,7 +1299,7 @@ static int tcmu_configure_device(struct se_device *dev) kref_get(&udev->kref); ret = tcmu_netlink_event(TCMU_CMD_ADDED_DEVICE, udev->uio_info.name, - udev->uio_info.uio_dev->minor); + udev->uio_info.uio_dev.minor); if (ret) goto err_netlink; @@ -1332,7 +1332,7 @@ static int tcmu_check_and_free_pending_cmd(struct tcmu_cmd *cmd) static bool tcmu_dev_configured(struct tcmu_dev *udev) { - return udev->uio_info.uio_dev ? true : false; + return !!(udev->se_dev.dev_flags & DF_CONFIGURED); } static void tcmu_blocks_release(struct tcmu_dev *udev) @@ -1381,7 +1381,7 @@ static void tcmu_free_device(struct se_device *dev) if (tcmu_dev_configured(udev)) { tcmu_netlink_event(TCMU_CMD_REMOVED_DEVICE, udev->uio_info.name, - udev->uio_info.uio_dev->minor); + udev->uio_info.uio_dev.minor); uio_unregister_device(&udev->uio_info); } diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index ff04b7f..7593db6 100644 --- a/drivers/uio/uio.c +++ b/drivers/uio/uio.c @@ -399,7 +399,7 @@ static void uio_free_minor(struct uio_device *idev) */ void uio_event_notify(struct uio_info *info) { - struct uio_device *idev = info->uio_dev; + struct uio_device *idev = &info->uio_dev; atomic_inc(&idev->event); wake_up_interruptible(&idev->wait); @@ -812,13 +812,8 @@ int __uio_register_device(struct module *owner, if (!parent || !info || !info->name || !info->version) return -EINVAL; - info->uio_dev = NULL; - - idev = devm_kzalloc(parent, sizeof(*idev), GFP_KERNEL); - if (!idev) { - return -ENOMEM; - } - + idev = &info->uio_dev; + memset(idev, 0, sizeof(*idev)); idev->owner = owner; idev->info = info; init_waitqueue_head(&idev->wait); @@ -841,8 +836,6 @@ int __uio_register_device(struct module *owner, if (ret) goto err_uio_dev_add_attributes; - info->uio_dev = idev; - if (info->irq && (info->irq != UIO_IRQ_CUSTOM)) { /* * Note that we deliberately don't use devm_request_irq @@ -879,10 +872,10 @@ void uio_unregister_device(struct uio_info *info) { struct uio_device *idev; - if (!info || !info->uio_dev) + if (!info) return; - idev = info->uio_dev; + idev = &info->uio_dev; uio_free_minor(idev); diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h index 3c85c81..4b1b1c8 100644 --- a/include/linux/uio_driver.h +++ b/include/linux/uio_driver.h @@ -95,7 +95,7 @@ struct uio_device { * @irqcontrol: disable/enable irqs when 0/1 is written to /dev/uioX */ struct uio_info { - struct uio_device *uio_dev; + struct uio_device uio_dev; const char *name; const char *version; struct uio_mem mem[MAX_UIO_MAPS];
It looks like there might be 2 issues with the uio_device allocation, or it looks like we are leaking the device for possibly a specific type of device case that I could not find but one of you may know about. Issues: 1. We use devm_kzalloc to allocate the uio_device, but the release function, devm_kmalloc_release, is just a noop, so the memory is never freed. 2. We are actually assigning the resource allocated by devm_kzalloc to the parent device and not the device that goes with the uio_device. If devm_kmalloc_release did free the memory, it would not actually be freed until the parent is which in a lot of cases is not until module removal, so there could have been thousands of uio_registe/unregister_device sequences. It seems like this is either a bug, or was done deliberately to support some type of device that needs that memory to not be freed. I checked upstream uio users, but did not see any type of device like this though. If this is a bug, this patch, made over Linus's tree, fixes the problems by just allocating the uio_device with the uio_info struct since they need to be allocated/freed/accessed at the same times. Signed-off-by: Mike Christie <mchristi@redhat.com> --- drivers/target/target_core_user.c | 6 +++--- drivers/uio/uio.c | 17 +++++------------ include/linux/uio_driver.h | 2 +- 3 files changed, 9 insertions(+), 16 deletions(-)