linux-kernel - Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once

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]

Message-ID: <fm7buc5wqjfbpkc4vciubjttk73k7vzahohlcolztrhjqywnca@pysupztheg6i>
Date: Wed, 19 Jun 2024 14:49:21 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Shakeel Butt <shakeel.butt@...ux.dev>
Cc: Andrew Morton <akpm@...ux-foundation.org>, 
	Michal Hocko <mhocko@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, 
	kernel-team@...a.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Kyle McMartin <kyle@...radead.org>
Subject: Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once

On Tue, Jun 18, 2024 at 02:34:21PM -0700, Shakeel Butt wrote:
> At the moment oversize kvmalloc warnings are triggered once using
> WARN_ON_ONCE() macro. One issue with this approach is that it only
> detects the first abuser and then ignores the remaining abusers which
> complicates detecting all such abusers in a timely manner. The situation
> becomes worse when the repro has low probability and requires production
> traffic and thus require large set of machines to find such abusers. In
> Mera production, this warn once is slowing down the detection of these
> abusers. Simply replace WARN_ON_ONCE with WARN_RATELIMIT.
> 
> Reported-by: Kyle McMartin <kyle@...radead.org>
> Signed-off-by: Shakeel Butt <shakeel.butt@...ux.dev>
> ---
>  mm/util.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 10f215985fe5..de36344e8d53 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -649,7 +649,8 @@ void *kvmalloc_node_noprof(size_t size, gfp_t flags, int node)
>  
>  	/* Don't even allow crazy sizes */
>  	if (unlikely(size > INT_MAX)) {
> -		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> +		WARN_RATELIMIT(!(flags & __GFP_NOWARN), "size = %zu > INT_MAX",
> +			       size);
>  		return NULL;
>  	}
>  

I don't think this is necessary. From the description I think interested
parties can get away with bpftrace.

Suppose you have an abuser of the sort and you are worried there is more
than one.

Then this one-liner will catch *all* of them, not just the ones which
were "lucky" to get logged with ratelimit:
bpftrace -e 'kprobe:kvmalloc_node_noprof /arg0 > 2147483647/ { @[kstack()] = count(); }'

Of course adding a probe is not free, but then again kvmalloc should not
be used often to begin with so I doubt it is going to have material
impact in terms of performance.

While I concede it takes more effort to get this running on all affected
machines, the result is much better than mere ratelimit. Also there is
no need to patch the kernel.

btw, I found drm keeps spamming kvmalloc, someone(tm) should look into
it:
@[
    kvmalloc_node_noprof+5
    drm_property_create_blob+76
    drm_atomic_helper_dirtyfb+234
    drm_fbdev_generic_helper_fb_dirty+509
    drm_fb_helper_damage_work+139
    process_one_work+376
    worker_thread+753
    kthread+207
    ret_from_fork+49
    ret_from_fork_asm+26
, 104]: 12