linux-kernel - Re: [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <199aebfb-f364-cd9b-5d2b-dbe42b779a41@intel.com>
Date:   Fri, 18 Feb 2022 11:54:54 +0100
From:   Andrzej Hajda <andrzej.hajda@...el.com>
To:     Eric Dumazet <edumazet@...gle.com>
CC:     LKML <linux-kernel@...r.kernel.org>,
        <intel-gfx@...ts.freedesktop.org>,
        <dri-devel@...ts.freedesktop.org>, netdev <netdev@...r.kernel.org>,
        "Jani Nikula" <jani.nikula@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        "Lucas De Marchi" <lucas.demarchi@...el.com>,
        Chris Wilson <chris.p.wilson@...el.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        "Jakub Kicinski" <kuba@...nel.org>
Subject: Re: [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing



On 17.02.2022 16:23, Eric Dumazet wrote:
> On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@...el.com> wrote:
>> In cases references are taken alternately on multiple exec paths leak
>> report can grow substantially, sorting and grouping leaks by stack_handle
>> allows to compact it.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@...el.com>
>> Reviewed-by: Chris Wilson <chris.p.wilson@...el.com>
>> ---
>>   lib/ref_tracker.c | 35 +++++++++++++++++++++++++++--------
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c
>> index 1b0c6d645d64a..0e9c7d2828ccb 100644
>> --- a/lib/ref_tracker.c
>> +++ b/lib/ref_tracker.c
>> @@ -1,5 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>   #include <linux/export.h>
>> +#include <linux/list_sort.h>
>>   #include <linux/ref_tracker.h>
>>   #include <linux/slab.h>
>>   #include <linux/stacktrace.h>
>> @@ -14,23 +15,41 @@ struct ref_tracker {
>>          depot_stack_handle_t    free_stack_handle;
>>   };
>>
>> +static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct list_head *b)
>> +{
>> +       const struct ref_tracker *ta = list_entry(a, const struct ref_tracker, head);
>> +       const struct ref_tracker *tb = list_entry(b, const struct ref_tracker, head);
>> +
>> +       return ta->alloc_stack_handle - tb->alloc_stack_handle;
>> +}
>> +
>>   void __ref_tracker_dir_print(struct ref_tracker_dir *dir,
>>                             unsigned int display_limit)
>>   {
>> +       unsigned int i = 0, count = 0;
>>          struct ref_tracker *tracker;
>> -       unsigned int i = 0;
>> +       depot_stack_handle_t stack;
>>
>>          lockdep_assert_held(&dir->lock);
>>
>> +       if (list_empty(&dir->list))
>> +               return;
>> +
>> +       list_sort(NULL, &dir->list, ref_tracker_cmp);
> What is going to be the cost of sorting a list with 1,000,000 items in it ?

Do we really have such cases?


>
> I just want to make sure we do not trade printing at most ~10 references
> (from netdev_wait_allrefs()) to a soft lockup :/ with no useful info
> if something went terribly wrong.
>
> I suggest that you do not sort a potential big list, and instead
> attempt to allocate an array of @display_limits 'struct stack_counts'
>
> I suspect @display_limits will always be kept to a reasonable value
> (less than 100 ?)

I though rather about 16 :)
In theory everything is possible, but do we have real case examples 
which could lead to 100 stack traces?
Maybe some frameworks used by multiple consumers (drivers) ???

>
> struct stack_counts {
>      depot_stack_handle_t stack_handle;
>      unsigned int count;
> }
>
> Then, iterating the list and update the array (that you can keep
> sorted by ->stack_handle)
>
> Then after iterating, print the (at_most) @display_limits handles
> found in the temp array.

OK, could be faster and less invasive.
Other solution would be keeping the array in dir and update in every 
tracker alloc/free, this way we avoid iteration over potentially big 
list, but it would cost memory and since printing is rather rare I am 
not sure if it is worth.

I will try your proposition.

Regards
Andrzej

>
>> +
>>          list_for_each_entry(tracker, &dir->list, head) {
>> -               if (i < display_limit) {
>> -                       pr_err("leaked reference.\n");
>> -                       if (tracker->alloc_stack_handle)
>> -                               stack_depot_print(tracker->alloc_stack_handle);
>> -                       i++;
>> -               } else {
>> +               if (i++ >= display_limit)
>>                          break;
>> -               }
>> +               if (!count++)
>> +                       stack = tracker->alloc_stack_handle;
>> +               if (stack == tracker->alloc_stack_handle &&
>> +                   !list_is_last(&tracker->head, &dir->list))
>> +                       continue;
>> +
>> +               pr_err("leaked %d references.\n", count);
>> +               if (stack)
>> +                       stack_depot_print(stack);
>> +               count = 0;
>>          }
>>   }
>>   EXPORT_SYMBOL(__ref_tracker_dir_print);
>> --
>> 2.25.1
>>