Re: [PATCH] mm: kmemleak: Treat vm_struct as alternative reference to vmalloc'ed objects

[Luis R. Rodriguez]

On Wed, May 24, 2017 at 9:57 AM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> On Tue, May 23, 2017 at 10:37:00PM +0200, Luis R. Rodriguez wrote:
>> On Mon, May 22, 2017 at 06:35:14PM +0100, Catalin Marinas wrote:
>> > Kmemleak requires that vmalloc'ed objects have a minimum reference count
>> > of 2: one in the corresponding vm_struct object and the other owned by
>> > the vmalloc() caller. There are cases, however, where the original
>> > vmalloc() returned pointer is lost and, instead, a pointer to vm_struct
>> > is stored (see free_thread_stack()). Kmemleak currently reports such
>> > objects as leaks.
>> >
>> > This patch adds support for treating any surplus references to an object
>> > as additional references to a specified object. It introduces the
>> > kmemleak_vmalloc() API function which takes a vm_struct pointer and sets
>> > its surplus reference passing to the actual vmalloc() returned pointer.
>> > The __vmalloc_node_range() calling site has been modified accordingly.
>> >
>> > An unrelated minor change is included in this patch to change the type
>> > of kmemleak_object.flags to unsigned int (previously unsigned long).
>> >
>> > Reported-by: "Luis R. Rodriguez" <mcgrof@xxxxxxxxxx>
>>
>> Tested-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
>
> Thanks.
>
>> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> > index 20036d4f9f13..11ab654502fd 100644
>> > --- a/mm/kmemleak.c
>> > +++ b/mm/kmemleak.c
>> > @@ -1188,6 +1249,30 @@ static bool update_checksum(struct kmemleak_object *object)
>> >  }
>> >
>> >  /*
>> > + * Update an object's references. object->lock must be held by the caller.
>> > + */
>> > +static void update_refs(struct kmemleak_object *object)
>> > +{
>> > +   if (!color_white(object)) {
>> > +           /* non-orphan, ignored or new */
>> > +           return;
>> > +   }
>> > +
>> > +   /*
>> > +    * Increase the object's reference count (number of pointers to the
>> > +    * memory block). If this count reaches the required minimum, the
>> > +    * object's color will become gray and it will be added to the
>> > +    * gray_list.
>> > +    */
>> > +   object->count++;
>> > +   if (color_gray(object)) {
>> > +           /* put_object() called when removing from gray_list */
>> > +           WARN_ON(!get_object(object));
>> > +           list_add_tail(&object->gray_list, &gray_list);
>> > +   }
>> > +}
>> > +
>> > +/*
>>
>> This an initial use of it seems to be very possible and likely without the
>> vmalloc special case, ie, can this be added as a separate patch to make the
>> actual functional change easier to read ?
>
> The above is just moving code from scan_block() into a separate
> function.

Exactly.

> But I'm happy to split this patch into 2-3 patches if it's
> easier to follow.

If it does cause a regression the block of code reverted would also be
smaller to revert / inspect.

  Luis