linux-kernel - Re: [PATCH] tracing: Fix overflow in get_free

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]

Message-ID: <b8a07b3e60ea173bdb1362dbe8e7034b4a1f25b6.camel@mediatek.com>
Date: Mon, 22 Jul 2024 05:55:12 +0000
From: Tze-nan Wu (吳澤南) <Tze-nan.Wu@...iatek.com>
To: "rostedt@...dmis.org" <rostedt@...dmis.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-trace-kernel@...r.kernel.org" <linux-trace-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	Cheng-Jui Wang (王正睿)
	<Cheng-Jui.Wang@...iatek.com>, Eric-YK Wu (吳育葵)
	<eric-yk.wu@...iatek.com>, wsd_upstream <wsd_upstream@...iatek.com>,
	Bobule Chang (張弘義) <bobule.chang@...iatek.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "mathieu.desnoyers@...icios.com"
	<mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] tracing: Fix overflow in get_free_elt()

On Wed, 2024-07-10 at 17:19 +0800, Tze-nan Wu wrote:
> "tracing_map->next_elt" in get_free_elt() is at risk of overflowing.
> 
> Once it overflows, new elements can still be inserted into the
> tracing_map
> even though the maximum number of elements (`max_elts`) has been
> reached.
> Continuing to insert elements after the overflow could result in the
> tracing_map containing "tracing_map->max_size" elements, leaving no
> empty
> entries.
> If any attempt is made to insert an element into a full tracing_map
> using
> `__tracing_map_insert()`, it will cause an infinite loop with
> preemption
> disabled, leading to a CPU hang problem.
> 
> Fix this by preventing any further increments to "tracing_map-
> >next_elt"
> once it reaches "tracing_map->max_elt".
> 
> Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@...iatek.com>
> Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@...iatek.com>
> Signed-off-by: Tze-nan Wu <Tze-nan.Wu@...iatek.com>
> ---
>  kernel/trace/tracing_map.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
Just a gentle ping. Any comments on this patch will be appreciated.
Actually we have encountered this issue internally after enabling the
throttle_rss_stat feature in Perfetto for an extended duration, during
which the rss_stat tracepoint was invoked over 2^32 times.
Then the CPU could hang in function "__tracing_map_insert()" after the
tracing_map left no empty entry.

throttle_rss_stat is literally:
1. $echo "rss_stat_throttled unsigned int mm_id unsigned int curr int
member long size" >> /sys/kernel/tracing/synthetic_events
2. $echo
'hist:keys=mm_id,member:bucket=size/0x80000:onchange($bucket).rss_stat_
throttled(mm_id,curr,member,size)' >
/sys/kernel/tracing/events/kmem/rss_stat/trigger

> diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
> index a4dcf0f24352..3a56e7c8aa4f 100644
> --- a/kernel/trace/tracing_map.c
> +++ b/kernel/trace/tracing_map.c
> @@ -454,7 +454,7 @@ static struct tracing_map_elt
> *get_free_elt(struct tracing_map *map)
>  	struct tracing_map_elt *elt = NULL;
>  	int idx;
>  
> -	idx = atomic_inc_return(&map->next_elt);
> +	idx = atomic_fetch_add_unless(&map->next_elt, 1, map-
> >max_elts);
>  	if (idx < map->max_elts) {
>  		elt = *(TRACING_MAP_ELT(map->elts, idx));
>  		if (map->ops && map->ops->elt_init)
> @@ -699,7 +699,7 @@ void tracing_map_clear(struct tracing_map *map)
>  {
>  	unsigned int i;
>  
> -	atomic_set(&map->next_elt, -1);
> +	atomic_set(&map->next_elt, 0);
>  	atomic64_set(&map->hits, 0);
>  	atomic64_set(&map->drops, 0);
>  
> @@ -783,7 +783,7 @@ struct tracing_map *tracing_map_create(unsigned
> int map_bits,
>  
>  	map->map_bits = map_bits;
>  	map->max_elts = (1 << map_bits);
> -	atomic_set(&map->next_elt, -1);
> +	atomic_set(&map->next_elt, 0);
>  
>  	map->map_size = (1 << (map_bits + 1));
>  	map->ops = ops;