| 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: <20070513192753.GA3014@tv-sign.ru>
Date: Sun, 13 May 2007 23:27:53 +0400
From: Oleg Nesterov <oleg@...sign.ru>
To: Tejun Heo <htejun@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Chinner <dgc@....com>,
David Howells <dhowells@...hat.com>,
Gautham Shenoy <ego@...ibm.com>,
Jarek Poplawski <jarkao2@...pl>, Ingo Molnar <mingo@...e.hu>,
Srivatsa Vaddagiri <vatsa@...ibm.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] make cancel_rearming_delayed_work() reliable
On 05/12, Tejun Heo wrote:
>
> Oleg Nesterov wrote:
> >
> > Yes I hate this barrier too. That is why changelog explicitly mentions it.
> > Probably we can do something else.
>
> Fortunately, we have one bit left in the flags and can use it to mark
> pointer validity instead of list_empty() test. flags and wq_data live
> in the same atomic_long and thus can be updated together atomically.
Heh. I thought about another bit-in-pointer too. I can't explain this,
but I personally hate these bits even more than barriers.
I must admit, your suggestion is much more clean compared to what I
had in mind.
> * insert_work() sets VALID bit and the cwq pointer using one
> atomic_long_set().
OK, but not very suitable, we need to copy-and-paste set_wq_data(),
because it is also used by queue_delayed_work(). Not a big problem.
> * queue_delayed_work_on() sets the cwq pointer but not the VALID bit.
... and queue_delayed_work(), of course.
> * run_workqueue() clears the cwq pointer and VALID bit while holding
> lock before executing the work.
We should not clear cwq. I guess you meant "does list_del() and clears
VALID bit".
> * try_to_grab_pending() checks VALID && pointers equal after grabbing
> cwq->lock.
We don't even need to check the pointers. VALID bit is always changed
under cwq->lock. So, if we see VALID under cwq->lock, we have a right
pointer.
> What do you think? Is there any hole?
I'd be happy to say I found many nasty unfixable holes in your idea,
but I can't see any :) Something like the patch below, I guess.
Ugh. OK, the barrier is evil. As I said, we can convert smp_wmb() to
smp_bm__before_spinlock() (which we don't currently have, but which we
imho need regardless to workqueue.c), but anyway it is not easy to
understand the code with barriers.
OTOH, the new VALID bit is _only_ needed to implement a special cancel_xxx
functions. And unlike wmb/smp_bm__before_spinlock it is not hidden in
insert_work(), but spreads over the queue/unqueue path. And. the
bit-in-pointer is always a hack, imho. And. It is a bit difficult to
understand why do we need a VALID bit when we have !list_empty(), so
_perhaps_ a barrier is not much worse.
I am looking at the patch below, and I can't undestand whether this is
good change or not. I am biased, of course. Tejun, if you say "I strongly
believe this is better", I'll send the patch.
Anybody else has an opinion?
Oleg.
--- t/kernel/workqueue.c~ 2007-05-13 22:32:45.000000000 +0400
+++ t/kernel/workqueue.c 2007-05-13 22:37:42.000000000 +0400
@@ -120,11 +120,7 @@ static void insert_work(struct cpu_workq
struct work_struct *work, int tail)
{
set_wq_data(work, cwq);
- /*
- * Ensure that we get the right work->data if we see the
- * result of list_add() below, see try_to_grab_pending().
- */
- smp_wmb();
+ __set_bit(WORK_STRUCT_QUEUED, work_data_bits(work));
if (tail)
list_add_tail(&work->entry, &cwq->worklist);
else
@@ -231,6 +227,12 @@ int queue_delayed_work_on(int cpu, struc
}
EXPORT_SYMBOL_GPL(queue_delayed_work_on);
+static inline void dequeue_work(struct work_struct *work)
+{
+ __clear_bit(WORK_STRUCT_QUEUED, work_data_bits(work));
+ list_del_init(&work->entry);
+}
+
static void run_workqueue(struct cpu_workqueue_struct *cwq)
{
spin_lock_irq(&cwq->lock);
@@ -246,8 +248,8 @@ static void run_workqueue(struct cpu_wor
struct work_struct, entry);
work_func_t f = work->func;
+ dequeue_work(work);
cwq->current_work = work;
- list_del_init(cwq->worklist.next);
spin_unlock_irq(&cwq->lock);
BUG_ON(get_wq_data(work) != cwq);
@@ -410,17 +412,9 @@ static int try_to_grab_pending(struct wo
return ret;
spin_lock_irq(&cwq->lock);
- if (!list_empty(&work->entry)) {
- /*
- * This work is queued, but perhaps we locked the wrong cwq.
- * In that case we must see the new value after rmb(), see
- * insert_work()->wmb().
- */
- smp_rmb();
- if (cwq == get_wq_data(work)) {
- list_del_init(&work->entry);
- ret = 1;
- }
+ if (test_bit(WORK_STRUCT_QUEUED, work_data_bits(work))) {
+ dequeue_work(work);
+ ret = 1;
}
spin_unlock_irq(&cwq->lock);
-
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