netdev - Re: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 10 Apr 2012 23:22:08 -0300
From:	Glauber Costa <glommer@...allels.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
CC:	<netdev@...r.kernel.org>, David Miller <davem@...emloft.net>,
	"Andrew Morton" <akpm@...ux-foundation.org>
Subject: Re: [PATCH] memcg/tcp: fix warning caused b res->usage go to negative.

> 
> The problem is that jump_label updating is not atomic_ops.

That's indeed really bad.
You seem to be right about that... I wonder, however, if we should
prevent anything to run in any cpu during the jump_label update?
The update can be slow, sure, but it is not expected to be frequent...

> I'm _not_ sure the update order of the jump_label in sock_update_memcg()
> and other jump instructions inserted at accounting.

Any ordering assumptions here would be extremely fragile, even if we
could make one.

> For example, if the jump instruction in sock_update_memcg() is updated 1st
> and others are updated later, it's unclear whether sockets which has _valid_
> sock->sk_cgrp will be accounted or not because accounting jump instruction
> may not be updated yet.
> 
> Hopefully, label in sock_update_memcg should be updated last...
> 
> Hm. If I do, I'll add one more key as:
> 
> atomic_t	sock_should_memcg_aware;
> 
> And update 2 keys in following order.
> 
> At enable
> 	static_key_slow_inc(&memcg_socket_limit_enabled)
> 	atomic_inc(&sock_should_memcg_aware);
> 
> At disable
> 	atomic_dec(&sock_should_memcg_aware);
> 	static_key_slow_dec(&memcg_socket_limit_enabled)
> 
> And
> ==
> void sock_update_memcg(struct sock *sk)
> {
>          if (atomic_read(&sock_should_memcg_aware)) {
> 
> ==

Problem here is that having an atomic variable here defeats a bit the
purpose of the jump labels.

If it is just in the update path, it is not *that* bad.

But unless we go fix the jump labels to prevent such thing from
happening, maybe it would be better to have two jump labels here?

One for the writer, that is updated last, and one from the readers.
This way we can force the ordering the way we want.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html