Linux Virtualization - Sep 2015 - DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna
wrote:
> The virtio core uses a static ida named virtio_index_ida for
> assigning index numbers to virtio devices during registration.
> The ida core may allocate some internal idr cache layers and
> an ida bitmap upon any ida allocation, and all these layers are
> truely freed only upon the ida destruction. The virtio_index_ida
> is not destroyed at present, leading to a memory leak when using
> the virtio core as a module and atleast one virtio device is
> registered and unregistered.
> 
> Fix this by invoking ida_destroy() in the virtio core module
> exit.
> 
> Cc: "Michael S. Tsirkin" <mst at redhat.com>
> Signed-off-by: Suman Anna <s-anna at ti.com>
Interesting.
Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?

If no, why not?

One doesn't generally expect to have to free global variables.
Maybe we should forbid DEFINE_IDA in modules?

James, could you comment on this please?
> ---
>  drivers/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index b1877d73fa56..7062bb0975a5 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -412,6 +412,7 @@ static int virtio_init(void)
>  static void __exit virtio_exit(void)
>  {
>  	bus_unregister(&virtio_bus);
> +	ida_destroy(&virtio_index_ida);
>  }
>  core_initcall(virtio_init);
>  module_exit(virtio_exit);
> -- 
> 2.5.0

On 09/17/2015 07:33 AM, Michael S. Tsirkin wrote:
> On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
>> The virtio core uses a static ida named virtio_index_ida for
>> assigning index numbers to virtio devices during registration.
>> The ida core may allocate some internal idr cache layers and
>> an ida bitmap upon any ida allocation, and all these layers are
>> truely freed only upon the ida destruction. The virtio_index_ida
>> is not destroyed at present, leading to a memory leak when using
>> the virtio core as a module and atleast one virtio device is
>> registered and unregistered.
>>
>> Fix this by invoking ida_destroy() in the virtio core module
>> exit.
>>
>> Cc: "Michael S. Tsirkin" <mst at redhat.com>
>> Signed-off-by: Suman Anna <s-anna at ti.com>
> 
> Interesting.
> Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
> 
> If no, why not?
> 
> One doesn't generally expect to have to free global variables.
> Maybe we should forbid DEFINE_IDA in modules?
> 
> James, could you comment on this please?
> 
Well, looking at the code 'ida_destroy' only need to be called
if you want/need to do a general cleanup.
It shouldn't be required if you do correct reference counting
on your objects, and call idr_remove() on each of them.

Unless I'm misreading something.

Seems like a topic for KS; Johannes had a larger patchset recently to
clean up idr, which run into very much the same issues.

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg)

On 09/17/2015 01:51 AM, Hannes Reinecke wrote:
> On 09/17/2015 07:33 AM, Michael S. Tsirkin wrote:
>> On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
>>> The virtio core uses a static ida named virtio_index_ida for
>>> assigning index numbers to virtio devices during registration.
>>> The ida core may allocate some internal idr cache layers and
>>> an ida bitmap upon any ida allocation, and all these layers are
>>> truely freed only upon the ida destruction. The virtio_index_ida
>>> is not destroyed at present, leading to a memory leak when using
>>> the virtio core as a module and atleast one virtio device is
>>> registered and unregistered.
>>>
>>> Fix this by invoking ida_destroy() in the virtio core module
>>> exit.
>>>
>>> Cc: "Michael S. Tsirkin" <mst at redhat.com>
>>> Signed-off-by: Suman Anna <s-anna at ti.com>
>>
>> Interesting.
>> Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
>> or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
Yes, I would think so as long as they are used as modules and the
modules are loaded & unloaded with a registration in between. For
built-in, it is not an issue.

A search on lkml yielded a similar fixup patches recently from Johannes
Thumshirn (actually those are using a idr, but both ida and idr use
common logic), see
https://patchwork.kernel.org/patch/6748601/ for one such patch.
>>
>> If no, why not?
>>
>> One doesn't generally expect to have to free global variables.
>> Maybe we should forbid DEFINE_IDA in modules?
>>
>> James, could you comment on this please?
>>
> Well, looking at the code 'ida_destroy' only need to be called
> if you want/need to do a general cleanup.
> It shouldn't be required if you do correct reference counting
> on your objects, and call idr_remove() on each of them.
> 
> Unless I'm misreading something.
Yeah, I should have written a better last sentence in the first para,
its not clear in the commit description without looking at my cover
letter. The memory leak is seen only when used as module and the module
is removed with atleast one ida allocation.

regards
Suman
> 
> Seems like a topic for KS; Johannes had a larger patchset recently to
> clean up idr, which run into very much the same issues.
> 
> Cheers,
> 
> Hannes
>