Messages in this thread Patch in this message | | | From | Thomas Gleixner <> | Subject | Re: [RFC V2] kthread: add object debug support | Date | Wed, 12 Aug 2020 12:27:03 +0200 |
| |
Stephen,
Stephen Boyd <sboyd@kernel.org> writes: > Quoting Qianli Zhao (2020-08-11 22:14:14) >> +/********** kernel/kthread **********/ >> +#define KWORK_ENTRY_STATIC ((void *) 0x600 + POISON_POINTER_DELTA) > > Is this related to the debugobjects change here? It looks like another > version of list poison.
Yes, it is. We use these poison entries to mark statically allocated objects. debug objects does not know about statically initialized objects up to the point where they are used (activated).
That means the object state lookup will fail which causes debugobjects to complain about using an uninitialized object. But in case of static initialized ones that's a false positive. So we mark these objects in their list head (or some other appropriate place) with a poison value and in case of a failed lookup debug object does:
if (descr->is_static_object && descr->is_static_object(addr)) { /* track this static object */ debug_object_init(addr, descr); debug_object_activate(addr, descr); }
The object specific is_static_object() callback will then check for the magic list poison value being present:
> +static bool kwork_is_static_object(void *addr) > +{ > + struct kthread_work *kwork = addr; > + > + return (kwork->node.prev == NULL && > + kwork->node.next == KWORK_ENTRY_STATIC); > +}
and if so the debug object core fixes its internal state by creating a tracking object and then activating it.
It's not a perfect "yes this is statically initialized" check but good enough. If you go and do:
work = kzalloc(sizeof(*work); work->node.next = KWORK_ENTRY_STATIC;
kthread_insert_work(worker, work);
or any other variant of insanity which makes the check claim that this is statically initialized then you rightfully can keep the pieces :)
>> diff --git a/kernel/kthread.c b/kernel/kthread.c >> index 132f84a..ca00bd2 100644 >> --- a/kernel/kthread.c >> +++ b/kernel/kthread.c >> @@ -698,6 +786,7 @@ int kthread_worker_fn(void *worker_ptr) >> work = list_first_entry(&worker->work_list, >> struct kthread_work, node); >> list_del_init(&work->node); >> + debug_kwork_deactivate(work); > > Shouldn't this come before the list operation so that any sort of fix > can be made before possibly corrupting a list?
Yes.
>> } >> worker->current_work = work; >> raw_spin_unlock_irq(&worker->lock); >> @@ -835,8 +924,11 @@ static void kthread_insert_work(struct kthread_worker *worker, >> >> list_add_tail(&work->node, pos); >> work->worker = worker; >> - if (!worker->current_work && likely(worker->task)) >> + >> + if (!worker->current_work && likely(worker->task)) { >> + debug_kwork_activate(work);
You missed this one as I explained to Qianli already. It's way worse than the other two.
Thanks,
tglx
| |