Re: [PATCH 1/1] uio: Fix uio_device memory leak
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
Re: [PATCH 1/1] uio: Fix uio_device memory leak
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.
Re: [PATCH 1/1] uio: Fix uio_device memory leak
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?
Re: [PATCH 1/1] uio: Fix uio_device memory leak
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
