Query:Regarding percpu_counter debug object destroy

Query:Regarding percpu_counter debug object destroy

[Kohli, Gaurav]

Hi ,

I have checked below code and it seems we are calling debug_object_free twice, ideally we should deactivate and later we
have to destroy.

1st call -> percpu_counter_destroy->debug_percpu_counter_deactivate -> debug_object_free
2nd call ->
debug_object_free



static bool percpu_counter_fixup_free(void *addr, enum debug_obj_state state)
{
         struct percpu_counter *fbc = addr;

         switch (state) {
         case ODEBUG_STATE_ACTIVE:
                 percpu_counter_destroy(fbc);  -> first call
                 debug_object_free(fbc, &percpu_counter_debug_descr); 2nd call
                 return true;
         default:
                 return false;
         }
}


We are seeing one issue, where one list contain garbage data in obj_hash, just before element of that is percpu_counter but
still not sure as it is very difficult to reproduce.

Can some one please review above code or can we remove one instance of debug_object_free from above code.

Regards
Gaurav

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: Query:Regarding percpu_counter debug object destroy

[Nikolay Borisov]

On 13.04.2018 10:32, Kohli, Gaurav wrote:
> Hi ,
> 
> I have checked below code and it seems we are calling debug_object_free
> twice, ideally we should deactivate and later we
> have to destroy.
> 
> 1st call -> percpu_counter_destroy->debug_percpu_counter_deactivate ->
> debug_object_free
> 2nd call ->
> debug_object_free
> 
> 
> 
> static bool percpu_counter_fixup_free(void *addr, enum debug_obj_state
> state)
> {
>         struct percpu_counter *fbc = addr;
> 
>         switch (state) {
>         case ODEBUG_STATE_ACTIVE:
>                 percpu_counter_destroy(fbc);  -> first call
>                 debug_object_free(fbc, &percpu_counter_debug_descr); 2nd


Having looked at the code I'd say this is indeed buggy. I'd say it
stemmed from same cargo culting since timer_fixup_free follows the same
structure of code, except that in del_timer_sync there is no code which
does debug_object_free. The situation is similar in work_fixup_free.

So at this point I guess the question is whether we want to leave the
debug_object_free call in percpu_counter_fixup_free and just remove
debug_percpu_counter_deactivate and open-code the call to
debug_object_deactivate in percpu_counter_destroy. Or remove the
explicit call in percpu_counter_fixup_free and leave
debug_percpu_counter_deactivate.


In the end it's a matter of style, so perhaps Tejun, as the maintainer,
has the final say what style he prefers. Personally, I'd go for the
former solution so that the percpu follows the style of the rest of the
kernel.

> call
>                 return true;
>         default:
>                 return false;
>         }
> }
> 
> 
> We are seeing one issue, where one list contain garbage data in
> obj_hash, just before element of that is percpu_counter but
> still not sure as it is very difficult to reproduce.
> 
> Can some one please review above code or can we remove one instance of
> debug_object_free from above code.
> 
> Regards
> Gaurav
> 

Re: Query:Regarding percpu_counter debug object destroy

[Kohli, Gaurav]

Hi Nikolay,

Thanks for the comment.

I agree ,like timer , hrtimer we have to mark inactive in destroy function and finally freeing the debug object

after destruction of percpu_counter.

But i am still not sure that this double freeing with same address may create race or not in debug_object list.

Regards

Gaurav

On 4/13/2018 1:12 PM, Nikolay Borisov wrote:

>
> On 13.04.2018 10:32, Kohli, Gaurav wrote:
>> Hi ,
>>
>> I have checked below code and it seems we are calling debug_object_free
>> twice, ideally we should deactivate and later we
>> have to destroy.
>>
>> 1st call -> percpu_counter_destroy->debug_percpu_counter_deactivate ->
>> debug_object_free
>> 2nd call ->
>> debug_object_free
>>
>>
>>
>> static bool percpu_counter_fixup_free(void *addr, enum debug_obj_state
>> state)
>> {
>>          struct percpu_counter *fbc = addr;
>>
>>          switch (state) {
>>          case ODEBUG_STATE_ACTIVE:
>>                  percpu_counter_destroy(fbc);  -> first call
>>                  debug_object_free(fbc, &percpu_counter_debug_descr); 2nd
>
> Having looked at the code I'd say this is indeed buggy. I'd say it
> stemmed from same cargo culting since timer_fixup_free follows the same
> structure of code, except that in del_timer_sync there is no code which
> does debug_object_free. The situation is similar in work_fixup_free.
>
> So at this point I guess the question is whether we want to leave the
> debug_object_free call in percpu_counter_fixup_free and just remove
> debug_percpu_counter_deactivate and open-code the call to
> debug_object_deactivate in percpu_counter_destroy. Or remove the
> explicit call in percpu_counter_fixup_free and leave
> debug_percpu_counter_deactivate.
>
>
> In the end it's a matter of style, so perhaps Tejun, as the maintainer,
> has the final say what style he prefers. Personally, I'd go for the
> former solution so that the percpu follows the style of the rest of the
> kernel.
>
>> call
>>                  return true;
>>          default:
>>                  return false;
>>          }
>> }
>>
>>
>> We are seeing one issue, where one list contain garbage data in
>> obj_hash, just before element of that is percpu_counter but
>> still not sure as it is very difficult to reproduce.
>>
>> Can some one please review above code or can we remove one instance of
>> debug_object_free from above code.
>>
>> Regards
>> Gaurav
>>
>>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.