Messages in this thread | | | Date | Sun, 18 May 2008 21:46:46 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH 3/3] posix timers: use SIGQUEUE_CANCELLED when the timer is destroyed |
| |
On 05/18, Linus Torvalds wrote: > > On Sun, 18 May 2008, Oleg Nesterov wrote: > > > > Initially, I did > > > > q->flags |= SIGQUEUE_CANCELLED; > > spin_lock_irqsave(lock, flags); > > q->flags &= ~SIGQUEUE_PREALLOC; > > > > to document the fact that SIGQUEUE_CANCELLED can be set lockless, but > > then "optimized" the code, couldn't help myself... Besides, the code > > above looks really confusing without the fat comment. > > Oh, and the above is *wrong*. > > Why? > > Becayse if SIGQUEUE_PREALLOC setting needs the lock, then setting any > *other* bit in that word will also need the lock! > > That's because > > q->flags |= SIGQUEUE_CANCELLED; > > writes those other bits too - admittedly with the value they were read > just before, but if it races with something setting SIGQUEUE_PREALLOC that > doesn't matter - the newly written version will simply be wrong. > > So the rule is that if one bit of a word needs locking, then they *all* > do.
Ah. I wasn't clear.
Clearing of SIGQUEUE_PREALLOC needs ->siglock, yes. But not because anybody else can write to q->flags. Nobody can, we (the timer) "own" this sigqueue.
Once we clear SIGQUEUE_PREALLOC, "q" can be freed by the receiver (it doesn't writes to q->flags, it only reads ->flags). After that we can't trust the list_empty() check, we just can't dereference this "struct sigqueue *".
Taking ->siglock before "&= ~SIGQUEUE_PREALLOC" ensures that "q" can't be be freed if it is queued, nothing more.
Oleg.
| |