| 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
| ||
|
Message-ID: <AANLkTikuKMLUoVnQsU6QNCnHBgOponJUngHGtyVHk1xK@mail.gmail.com>
Date: Mon, 20 Dec 2010 17:28:02 +0800
From: Yong Zhang <yong.zhang0@...il.com>
To: Andy Walls <awalls@...metrocast.net>
Cc: linux-kernel@...r.kernel.org, nicolas.mailhot@...oste.net,
Tejun Heo <tj@...nel.org>, Jarod Wilson <jarod@...hat.com>,
Ingo Molnar <mingo@...hat.com>,
Mauro Carvalho Chehab <mchehab@...hat.com>,
Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH] kthread_worker: Initialize dynamically allocated spinlock
properly for lockdep
On Mon, Dec 20, 2010 at 3:07 PM, Yong Zhang <yong.zhang0@...il.com> wrote:
> On Sun, Dec 19, 2010 at 8:49 PM, Andy Walls <awalls@...metrocast.net> wrote:
>> init_kthread_worker(), via KTHREAD_WORKER_INIT(), used an
>> initializer for static spin_lock objects, SPIN_LOCK_UNLOCKED, on
>> a dynamically allocated kthread_worker object's internal spinlock_t.
>> This causes lockdep to gripe:
>>
>> INFO: trying to register non-static key.
>> the code is fine but needs lockdep annotation.
>> turning off the locking correctness validator.
>>
>> To keep lockdep happy, use spin_lock_init() for dynamically
>> allocated kthread_worker objects' internal spinlock_t.
>>
>> Reported-by: Nicolas <nicolas.mailhot@...oste.net>
>> Signed-off-by: Andy Walls <awalls@...metrocast.net>
>>
>> Cc: Tejun Heo <tj@...nel.org>
>> Cc: Jarod Wilson <jarod@...hat.com>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Mauro Carvalho Chehab <mchehab@...hat.com>
>> Cc: Hans Verkuil <hverkuil@...all.nl>
>>
>> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
>> index 685ea65..e65d0b1 100644
>> --- a/include/linux/kthread.h
>> +++ b/include/linux/kthread.h
>> @@ -83,7 +83,13 @@ struct kthread_work {
>>
>> static inline void init_kthread_worker(struct kthread_worker *worker)
>> {
>> - *worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
>> + /*
>> + * Lockdep complains if a dynamically allocated worker's spinlock_t
>> + * is initialzed using SPIN_LOCK_UNLOCKED.
>> + */
>> + spin_lock_init(&worker->lock);
>
> This will make different kthead_worker->lock initialized with one same
> key. You know spin_lock_init() will be like below:
> # define raw_spin_lock_init(lock) \
> do { \
> static struct lock_class_key __key; \
> \
> __raw_spin_lock_init((lock), #lock, &__key); \
> } while (0)
>
> So we should put the real initializer to kernel/kthread.c
> and make init_kthread_worker() to be a MACRO.
>
> BTW, init_kthread_work() should also be changed like above
> because member done is a wait_queue_head.
untested patch is here. Andy/Nicolas, is it ok for you?
---
Subject: [PATCH] kthread_work: Make lockdep happy
spinlock in kthread_worker and wait_queue_head in kthread_work
both should be lockdep annotated.
So change the interface to make it suiltable for CONFIG_LOCKDEP.
Signed-off-by: Yong Zhang <yong.zhang0@...il.com>
---
I'm not sure if it's possible to define a worker on stack?
So I left DEFINE_KTHREAD_WORKER() untouched.
include/linux/kthread.h | 37 +++++++++++++++++++++++++++----------
kernel/kthread.c | 9 +++++++++
2 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 685ea65..5d516b3 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -75,22 +75,39 @@ struct kthread_work {
.flushing = ATOMIC_INIT(0), \
}
+/* Is it possible to define a worker on stack? */
#define DEFINE_KTHREAD_WORKER(worker) \
struct kthread_worker worker = KTHREAD_WORKER_INIT(worker)
#define DEFINE_KTHREAD_WORK(work, fn) \
struct kthread_work work = KTHREAD_WORK_INIT(work, fn)
-static inline void init_kthread_worker(struct kthread_worker *worker)
-{
- *worker = (struct kthread_worker)KTHREAD_WORKER_INIT(*worker);
-}
-
-static inline void init_kthread_work(struct kthread_work *work,
- kthread_work_func_t fn)
-{
- *work = (struct kthread_work)KTHREAD_WORK_INIT(*work, fn);
-}
+#ifdef CONFIG_LOCKDEP
+# define KTHREAD_WORK_INIT_ONSTACK(work, fn) \
+ ({init_kthread_work((&work), fn); work})
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) \
+ struct kthread_work work = KTHREAD_WORK_INIT_ONSTACK(work, fn)
+#else
+# define DEFINE_KTHREAD_WORK_ONSTACK(work, fn) DEFINE_KTHREAD_WORK(work, fn)
+#endif
+
+extern void __init_kthread_worker(struct kthread_worker *worker,
+ struct lock_class_key *key);
+
+#define init_kthread_worker(worker) \
+ do { \
+ static struct lock_class_key __key; \
+ __init_kthread_worker((worker), &__key); \
+ } while (0)
+
+#define init_kthread_work(work, fn) \
+ do { \
+ memset((work), 0, sizeof(struct kthread_work)); \
+ INIT_LIST_HEAD(&(work)->node); \
+ (work)->func = (fn); \
+ init_waitqueue_head(&(work)->done); \
+ (work)->flushing = ATOMIC_INIT(0); \
+ } while (0)
int kthread_worker_fn(void *worker_ptr);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 2dc3786..fae2eff 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -265,6 +265,15 @@ int kthreadd(void *unused)
return 0;
}
+void __init_kthread_worker(struct kthread_worker *worker,
+ struct lock_class_key *key)
+{
+ spin_lock_init(&worker->lock);
+ lockdep_set_class(&worker->lock, key);
+ INIT_LIST_HEAD(&worker->work_list);
+ worker->task == NULL;
+}
+
/**
* kthread_worker_fn - kthread function to process kthread_worker
* @worker_ptr: pointer to initialized kthread_worker
--
1.7.0.4
--
Only stand for myself.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists