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

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

[Michael S. Tsirkin]

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@redhat.com>
> Signed-off-by: Suman Anna <s-anna@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

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

[Hannes Reinecke]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> 
> 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-l3A5Bk7waGM@public.gmane.org			      +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)

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

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

[Suman Anna]

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@redhat.com>
>>> Signed-off-by: Suman Anna <s-anna@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
> 

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

[James Bottomley]

On Thu, 2015-09-17 at 08:33 +0300, 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> 
> 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?

ida is Tejun's baby (cc'd).  However, it does look like without
ida_destroy() you will leave a cached ida->bitmap dangling because we're
trying to be a bit clever in ida_remove() so we cache the bitmap to
relieve ida_pre_get() of the burden if we would otherwise free it.

I don't understand why you'd want to forbid DEFINE_IDA ... all it does
is pre-initialise a usually static ida structure.  The initialised
structure will have a NULL bitmap cache that's allocated in the first
ida_pre_get() ... that all seems to work as expected and no different
from a dynamically allocated struct ida.  Or are you thinking because
ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
which case I'm not sure there's much benefit to making it reusable, but
I suppose we could by adding a memset into ida_destroy().

James

> > ---
> >  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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 




-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

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

[Tejun Heo]

Hello,

On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> I don't understand why you'd want to forbid DEFINE_IDA ... all it does

I guess to require the use of explicit init / creation so that it's
clear the data structure needs to be destroyed?

> is pre-initialise a usually static ida structure.  The initialised
> structure will have a NULL bitmap cache that's allocated in the first
> ida_pre_get() ... that all seems to work as expected and no different
> from a dynamically allocated struct ida.  Or are you thinking because
> ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
> which case I'm not sure there's much benefit to making it reusable, but
> I suppose we could by adding a memset into ida_destroy().

I don't know.  Data structures which do lazy anything would likely
need explicit destruction and I'm not sure we'd wanna ban static
initialization for all such cases.  Seems like an unnecessary
restriction.

Thanks.

-- 
tejun

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

[Michael S. Tsirkin]

On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> On Thu, 2015-09-17 at 08:33 +0300, 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@redhat.com>
> > > Signed-off-by: Suman Anna <s-anna@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?
> 
> ida is Tejun's baby (cc'd).  However, it does look like without
> ida_destroy() you will leave a cached ida->bitmap dangling because we're
> trying to be a bit clever in ida_remove() so we cache the bitmap to
> relieve ida_pre_get() of the burden if we would otherwise free it.
> 
> I don't understand why you'd want to forbid DEFINE_IDA ... all it does
> is pre-initialise a usually static ida structure.  The initialised
> structure will have a NULL bitmap cache that's allocated in the first
> ida_pre_get() ... that all seems to work as expected and no different
> from a dynamically allocated struct ida.  Or are you thinking because
> ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
> which case I'm not sure there's much benefit to making it reusable, but
> I suppose we could by adding a memset into ida_destroy().
> 
> James

It's just unusual to have  a descructor without a constructor.
I bet more drivers misuse this AI because of this.

> > > ---
> > >  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
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> 

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

[James Bottomley]

On Thu, 2015-09-17 at 19:06 +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> > On Thu, 2015-09-17 at 08:33 +0300, 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > > Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
> > > 
> > > 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?
> > 
> > ida is Tejun's baby (cc'd).  However, it does look like without
> > ida_destroy() you will leave a cached ida->bitmap dangling because we're
> > trying to be a bit clever in ida_remove() so we cache the bitmap to
> > relieve ida_pre_get() of the burden if we would otherwise free it.
> > 
> > I don't understand why you'd want to forbid DEFINE_IDA ... all it does
> > is pre-initialise a usually static ida structure.  The initialised
> > structure will have a NULL bitmap cache that's allocated in the first
> > ida_pre_get() ... that all seems to work as expected and no different
> > from a dynamically allocated struct ida.  Or are you thinking because
> > ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
> > which case I'm not sure there's much benefit to making it reusable, but
> > I suppose we could by adding a memset into ida_destroy().
> > 
> > James
> 
> It's just unusual to have  a descructor without a constructor.
> I bet more drivers misuse this AI because of this.


Well, there's an easy fix for that.  We could have ida_remove() actually
free the bitmap and not cache it if it's the last layer.  That way ida
would naturally empty and we wouldn't need a destructor.   Tejun, would
that work?

James

> > > > ---
> > > >  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
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

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

[Tejun Heo]

Hello,

On Thu, Sep 17, 2015 at 09:48:37AM -0700, James Bottomley wrote:
> Well, there's an easy fix for that.  We could have ida_remove() actually
> free the bitmap and not cache it if it's the last layer.  That way ida
> would naturally empty and we wouldn't need a destructor.   Tejun, would
> that work?

Yeah, that definitely is one way to go about it.  It kinda muddles the
purpose of ida_destroy() tho.  I suppose we can rename it to
idr_remove_all() and then do the same to idr.  I'm not particularly
objecting to all that but what's wrong with just calling idr_destroy()
on exit paths?  If missing the call in modules is an issue, maybe we
can just annotate idr/ida with debugobj?

Thanks.

-- 
tejun

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

[James Bottomley]

On Thu, 2015-09-17 at 13:15 -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, Sep 17, 2015 at 09:48:37AM -0700, James Bottomley wrote:
> > Well, there's an easy fix for that.  We could have ida_remove() actually
> > free the bitmap and not cache it if it's the last layer.  That way ida
> > would naturally empty and we wouldn't need a destructor.   Tejun, would
> > that work?
> 
> Yeah, that definitely is one way to go about it.  It kinda muddles the
> purpose of ida_destroy() tho.  I suppose we can rename it to
> idr_remove_all() and then do the same to idr.  I'm not particularly
> objecting to all that but what's wrong with just calling idr_destroy()
> on exit paths?  If missing the call in modules is an issue, maybe we
> can just annotate idr/ida with debugobj?

The argument is that we shouldn't have to explicitly destroy a
statically initialized object, so 

DEFINE_IDA(someida);

Should just work without having to explicitly do

ida_destory(someida);

somewhere in the exit code.  It's about usage patterns.  Michael's
argument is that if we can't follow the no destructor pattern for
DEFINE_IDA() then we shouldn't have it at all, because it's confusing
kernel design patterns.  The pattern we would have would be

struct ida someida:

ida_init(&someida);

...

ida_destroy(&someida);

so the object explicitly has a constructor matched to a destructor.

James


-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

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

[Tejun Heo]

Hello, James.

On Thu, Sep 17, 2015 at 10:58:29AM -0700, James Bottomley wrote:
> The argument is that we shouldn't have to explicitly destroy a
> statically initialized object, so 
> 
> DEFINE_IDA(someida);
> 
> Should just work without having to explicitly do
> 
> ida_destory(someida);
> 
> somewhere in the exit code.  It's about usage patterns.  Michael's
> argument is that if we can't follow the no destructor pattern for
> DEFINE_IDA() then we shouldn't have it at all, because it's confusing
> kernel design patterns.  The pattern we would have would be
> 
> struct ida someida:
> 
> ida_init(&someida);
> 
> ...
> 
> ida_destroy(&someida);
> 
> so the object explicitly has a constructor matched to a destructor.

Yeah, I get that.  I'm just not convinced that this matters enough
especially if we can get debugobj/ksan/whatever trip on it.

Thanks.

-- 
tejun