lkml.org 
[lkml]   [2009]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [GIT PULL] percpu fixes for 2.6.32-rc6


On Tue, 10 Nov 2009, Tejun Heo wrote:
>
> Please pull from the following percpu fix branch.

No way in hell.

> It fixes a possible deadlock caused by lock ordering inversion through
> irq.

.. and it does so by introducing a new bug. No thank you.

> +
> + /*
> + * pcpu_mem_free() might end up calling vfree() which uses
> + * IRQ-unsafe lock and thus can't be called with pcpu_lock
> + * held. Release and reacquire pcpu_lock if old map needs to
> + * be freed.
> + */
> + if (old) {
> + spin_unlock_irqrestore(&pcpu_lock, *flags);
> + pcpu_mem_free(old, size);
> + spin_lock_irqsave(&pcpu_lock, *flags);
> + }

Routines that drop and then re-take the lock should be banned, as it's
almost always a bug waiting to happen. As it is this time:

> return 0;

Now the caller will happily continue to traverse a list that may no longer
be valid, because you dropped the lock.

Really. This thing is total sh*t. It was misdesigned to start with, and
the calling convention is wrong. That 'pcpu_extend_area_map()' function
should be split up into two functions: 'pcpu_needs_to_extend()' that never
drops the lock, and 'pcpu_extend_area()' that _always_ drops the lock
(and then returns an error if it can't allocate the memory).

Not that shit-for-brains that may or may not drop the lock, and then
returns an incorrect error return depending on whether it did.

In other words: fix the sh*t, don't add even more to it. That 'return 0'
was and is wrong. It should have been a 'return 1'. And thank the Gods
that I looked at it,

Sure, you can fix the bug by just returning 1. But you can't fix the total
crap of a calling convention that way. Fix it properly as outlined above,
and remember: functions that drop locks that were held when called are
EVIL and almost always the source of really subtle races.

As it was in this case.

Linus


\
 
 \ /
  Last update: 2009-11-18 23:46    [W:0.050 / U:1.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site