Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Stephen Boyd <sboyd@kernel.org>, Felix.Kuehling@amd.com,
        Qianli Zhao <zhaoqianligood@gmail.com>,
        akpm@linux-foundation.org, axboe@kernel.dk
Cc:     john.stultz@linaro.org, ben.dooks@codethink.co.uk,
        bfields@redhat.com, cl@rock-chips.com,
        linux-kernel@vger.kernel.org, zhaoqianli@xiaomi.com
Subject: Re: [RFC V2] kthread: add object debug support
In-Reply-To: <159722125596.33733.17725649536425524344@swboyd.mtv.corp.google.com>
References: <311159bc826dcca2848344fc277c0069cff0a164.1597207603.git.zhaoqianli@xiaomi.com> <159722125596.33733.17725649536425524344@swboyd.mtv.corp.google.com>
Date:   Wed, 12 Aug 2020 12:27:03 +0200
Message-ID: <87pn7w5fd4.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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