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.