linux-kernel - Re: [PATCH v4 1/3] make jump_labels wait while updates are in place

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 27 Apr 2012 11:59:37 -0300
From:	Glauber Costa <glommer@...allels.com>
To:	Jason Baron <jbaron@...hat.com>
CC:	Steven Rostedt <rostedt@...dmis.org>, <cgroups@...r.kernel.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Li Zefan <lizefan@...wei.com>, Tejun Heo <tj@...nel.org>,
	<kamezawa.hiroyu@...fujitsu.com>, <linux-mm@...ck.org>,
	<devel@...nvz.org>, Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH v4 1/3] make jump_labels wait while updates are in place

On 04/27/2012 10:53 AM, Jason Baron wrote:
> On Thu, Apr 26, 2012 at 08:43:06PM -0400, Steven Rostedt wrote:
>> On Thu, Apr 26, 2012 at 07:51:05PM -0300, Glauber Costa wrote:
>>> In mem cgroup, we need to guarantee that two concurrent updates
>>> of the jump_label interface wait for each other. IOW, we can't have
>>> other updates returning while the first one is still patching the
>>> kernel around, otherwise we'll race.
>>
>> But it shouldn't. The code as is should prevent that.
>>
>>>
>>> I believe this is something that can fit well in the static branch
>>> API, without noticeable disadvantages:
>>>
>>> * in the common case, it will be a quite simple lock/unlock operation
>>> * Every context that calls static_branch_slow* already expects to be
>>>    in sleeping context because it will mutex_lock the unlikely case.
>>> * static_key_slow_inc is not expected to be called in any fast path,
>>>    otherwise it would be expected to have quite a different name. Therefore
>>>    the mutex + atomic combination instead of just an atomic should not kill
>>>    us.
>>>
>>> Signed-off-by: Glauber Costa<glommer@...allels.com>
>>> CC: Tejun Heo<tj@...nel.org>
>>> CC: Li Zefan<lizefan@...wei.com>
>>> CC: Kamezawa Hiroyuki<kamezawa.hiroyu@...fujitsu.com>
>>> CC: Johannes Weiner<hannes@...xchg.org>
>>> CC: Michal Hocko<mhocko@...e.cz>
>>> CC: Ingo Molnar<mingo@...e.hu>
>>> CC: Jason Baron<jbaron@...hat.com>
>>> ---
>>>   kernel/jump_label.c |   21 +++++++++++----------
>>>   1 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
>>> index 4304919..5d09cb4 100644
>>> --- a/kernel/jump_label.c
>>> +++ b/kernel/jump_label.c
>>> @@ -57,17 +57,16 @@ static void jump_label_update(struct static_key *key, int enable);
>>>
>>>   void static_key_slow_inc(struct static_key *key)
>>>   {
>>> +	jump_label_lock();
>>>   	if (atomic_inc_not_zero(&key->enabled))
>>> -		return;
>>
>> If key->enabled is not zero, there's nothing to be done. As the jump
>> label has already been enabled. Note, the key->enabled doesn't get set
>> until after the jump label is updated. Thus, if two tasks were to come
>> in, they both would be locked on the jump_label_lock().
>>
>

Okay, we seem to have been tricked by the usage of atomic while 
analyzing this. The fact that the atomic update happens after the code 
is patched seems enough to guarantee what we need, now that I read it 
again (and it seems so obvious =p )
--
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/