| 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
| ||
|
Date: Fri, 2 Feb 2018 11:52:50 -0800
From: Yang Shi <yang.shi@...ux.alibaba.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: longman@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2 v5] lib: debugobjects: handle objects free in a batch
outside the loop
On 2/1/18 3:01 PM, Yang Shi wrote:
>
>
> On 2/1/18 1:36 PM, Thomas Gleixner wrote:
>> On Fri, 2 Feb 2018, Yang Shi wrote:
>>
>>> /*
>>> - * Allocate a new object. If the pool is empty, switch off the
>>> debugger.
>>> + * Allocate a new object. Retrieve from global freelist first. If
>>> the pool is
>>> + * empty, switch off the debugger.
>>> * Must be called with interrupts disabled.
>>> */
>>> static struct debug_obj *
>>> @@ -150,6 +154,13 @@ static struct debug_obj *lookup_object(void
>>> *addr, struct debug_bucket *b)
>>> struct debug_obj *obj = NULL;
>>> raw_spin_lock(&pool_lock);
>> Why in alloc_object() and not in fill_pool()?
>>
>>> + if (obj_nr_tofree > 0 && (obj_pool_free < obj_pool_min_free)) {
>>> + obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>>> + obj_nr_tofree--;
>>> + hlist_del(&obj->node);
>>> + goto out;
>>> + }
>> Errm. This is wrong. It does not reinitialize the object. Please do that
>> shuffling in fill_pool().
>
> OK, will move the reuse logic into fill_pool().
>
>>
>>> if (obj_pool.first) {
>>> obj = hlist_entry(obj_pool.first, typeof(*obj), node);
>> ....
>>
>>> + /* When pool list is not full move free objs to pool list */
>>> + while (obj_pool_free < debug_objects_pool_size) {
>>> + if (obj_nr_tofree <= 0)
>>> + break;
>>> +
>>> + obj = hlist_entry(obj_to_free.first, typeof(*obj), node);
>>> + hlist_del(&obj->node);
>>> + hlist_add_head(&obj->node, &obj_pool);
>>> + obj_pool_free++;
>>> + obj_pool_used--;
>>> + obj_nr_tofree--;
>>> + }
>>> +
>>> + /*
>>> + * pool list is already full, and there are still objs on the
>>> free list,
>>> + * move remaining free objs to a separate list to free the
>>> memory later.
>>> + */
>>> + if (obj_nr_tofree > 0) {
>>> + hlist_move_list(&obj_to_free, &tofree);
>>> + obj_nr_tofree = 0;
>>> + }
>> The accounting is inconsistent. You leak obj_pool_used. But that's wrong
>> anyway because an object should not be accounted for in two places. It's
>> only on _ONE_ list....
>
> So I should move the accounting to where the obj is deleted from the
> list? It should look like:
I got your point here. Yes, obj_pool_used should be not decreased here
since it has not been allocated from pool list.
But, I think obj_nr_tofree counter should be cleared since all the objs
are *NOT* on the global free list anymore. They will be freed later.
And, we can't decrease the obj_nr_tofree counter later without acquiring
pool lock.
>
> if (obj_nr_tofree > 0)
> hlist_move_list(&obj_to_free, &tofree);
>
> ...
>
> if (!hlist_empty(&tofree)) {
> hlist_for_each_entry_safe(obj, tmp, &tofree, node) {
> hlist_del(&obj->node);
> obj_nr_tofree--;
> kmem_cache_free(obj_cache, obj);
> }
> }
>
>>
>>> @@ -716,7 +762,6 @@ static void __debug_check_no_obj_freed(const
>>> void *address, unsigned long size)
>>> {
>>> unsigned long flags, oaddr, saddr, eaddr, paddr, chunks;
>>> struct hlist_node *tmp;
>>> - HLIST_HEAD(freelist);
>>> struct debug_obj_descr *descr;
>>> enum debug_obj_state state;
>>> struct debug_bucket *db;
>>> @@ -752,18 +797,17 @@ static void __debug_check_no_obj_freed(const
>>> void *address, unsigned long size)
>>> goto repeat;
>>> default:
>>> hlist_del(&obj->node);
>>> - hlist_add_head(&obj->node, &freelist);
>>> + /* Put obj on the global free list */
>>> + raw_spin_lock(&pool_lock);
>>> + hlist_add_head(&obj->node, &obj_to_free);
>>> + /* Update the counter of objs on the global
>>> freelist */
>>> + obj_nr_tofree++;
>>> + raw_spin_unlock(&pool_lock);
>> As we have to take pool_lock anyway, we simply can change
>> free_object() to:
>>
>> static bool __free_object(obj)
>> {
>> bool work;
>>
>> lock(pool);
>> work = obj_pool_free > debug_objects_pool_size && obj_cache;
>> obj_pool_used++;
Should it be decreased here since the obj is being dequeued from hlist?
Thanks,
Yang
>> if (work) {
>> obj_nr_tofree++;
>> hlist_add_head(&obj->node, &obj_to_free);
>> ] else {
>> obj_pool_free++;
>> hlist_add_head(&obj->node, &obj_pool);
>> }
>> unlock(pool);
>> return work;
>> }
>>
>> static void free_object(obj)
>> {
>> if (__free_object(obj))
>> schedule_work(&debug_obj_work);
>> }
>>
>> and then use __free_object() in __debug_check_no_obj_freed()
>>
>> bool work = false;
>>
>> ...
>> work |= __free_object(obj);
>> ...
>>
>> if (work)
>> schedule_work(&debug_obj_work);
>>
>> That makes the whole thing simpler and the accounting is matching the
>> place
>> where the object is:
>>
>> obj_pool_free counts the number of objects enqueued in obj_pool
>> obj_nr_tofree counts the number of objects enqueued in
>> obj_to_free
>> obr_pool_used counts the number of objects enqueued in the
>> hash lists
>>
>> Ideally you split that patch into pieces:
>>
>> 1) Introduce obj_to_free/obj_nr_tofree and add the removing/freeing
>> from it
>> in fill_pool() and free_obj_work(). Nothing adds an object at
>> this point
>> to obj_to_free.
>>
>> 2) Change free_object() to use obj_to_free and split it apart
>>
>> 3) Change __debug_check_no_obj_freed() to use __free_object()
>>
>> That makes it simpler to review and to follow.
>>
>> Hmm?
>
> Sure, will refactor free_object() and split the patches in newer version.
>
> Thanks,
> Yang
>
>>
>> Thanks,
>>
>> tglx
Powered by blists - more mailing lists