* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-18 21:34 [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once Shakeel Butt
@ 2024-06-18 21:40 ` Linus Torvalds
2024-06-18 21:44 ` Shakeel Butt
2024-06-19 7:19 ` Michal Hocko
2024-06-19 12:49 ` Mateusz Guzik
2 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2024-06-18 21:40 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Michal Hocko, kernel-team, linux-mm, linux-kernel,
Kyle McMartin
On Tue, 18 Jun 2024 at 14:34, Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> Simply replace WARN_ON_ONCE with WARN_RATELIMIT.
NAK.
Sadly, the RATELIMIT cases are useless.
The normal rate limiting is basically "burst of up to ten, every five seconds".
That's going to completely swamp things and hide any other issue.
If we ratelimit it to "at most 1 per hour", maybe something like that
would be acceptable.
But honestly, I do not understand your "first abuser only" complaint.
There should not be *any* abusers. So just fix that first one already.
If you have more than one, you have bigger issues. So what is the real
reason for this broken patch? Why didn't you fix the first one?
Linus
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-18 21:40 ` Linus Torvalds
@ 2024-06-18 21:44 ` Shakeel Butt
0 siblings, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2024-06-18 21:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Michal Hocko, kernel-team, linux-mm, linux-kernel,
Kyle McMartin
On Tue, Jun 18, 2024 at 02:40:10PM GMT, Linus Torvalds wrote:
> On Tue, 18 Jun 2024 at 14:34, Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > Simply replace WARN_ON_ONCE with WARN_RATELIMIT.
>
> NAK.
>
> Sadly, the RATELIMIT cases are useless.
>
> The normal rate limiting is basically "burst of up to ten, every five seconds".
>
> That's going to completely swamp things and hide any other issue.
>
> If we ratelimit it to "at most 1 per hour", maybe something like that
> would be acceptable.
"at most 1 per hour" sounds good.
>
> But honestly, I do not understand your "first abuser only" complaint.
> There should not be *any* abusers. So just fix that first one already.
>
> If you have more than one, you have bigger issues. So what is the real
> reason for this broken patch? Why didn't you fix the first one?
>
The issue is the turnaround time to fix the first abuser and deploy the
fixed kernel to big enough fleet to find the remaining abusers. Usually
this turnaround time is in months.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-18 21:34 [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once Shakeel Butt
2024-06-18 21:40 ` Linus Torvalds
@ 2024-06-19 7:19 ` Michal Hocko
2024-06-19 8:03 ` Shakeel Butt
2024-06-19 12:49 ` Mateusz Guzik
2 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2024-06-19 7:19 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Tue 18-06-24 14:34:21, 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.
Long time ago, I've had a patch to do the once_per_callsite WARN. I
cannot find reference at the moment but it used stack depot to note
stacks that have already triggered. Back then there was no reponse on
the ML. Should I try to dig deep and recover it from my archives? I
think this is exactly kind of usecase where it would fit.
> Reported-by: Kyle McMartin <kyle@infradead.org>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.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;
> }
>
> --
> 2.43.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 7:19 ` Michal Hocko
@ 2024-06-19 8:03 ` Shakeel Butt
2024-06-19 8:30 ` Michal Hocko
0 siblings, 1 reply; 14+ messages in thread
From: Shakeel Butt @ 2024-06-19 8:03 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Wed, Jun 19, 2024 at 09:19:41AM GMT, Michal Hocko wrote:
> On Tue 18-06-24 14:34:21, 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.
>
> Long time ago, I've had a patch to do the once_per_callsite WARN. I
> cannot find reference at the moment but it used stack depot to note
> stacks that have already triggered. Back then there was no reponse on
> the ML. Should I try to dig deep and recover it from my archives? I
> think this is exactly kind of usecase where it would fit.
>
Do you mean something like warn once per unique call stack? If yes then
I think that is better than the simple ratelimiting version as
ratelimiting one may still miss some abusers and also may keep warning
about the same abuser. Please do share your patch.
Thanks,
Shakeel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 8:03 ` Shakeel Butt
@ 2024-06-19 8:30 ` Michal Hocko
2024-06-19 8:37 ` Shakeel Butt
2024-06-19 8:48 ` Michal Hocko
0 siblings, 2 replies; 14+ messages in thread
From: Michal Hocko @ 2024-06-19 8:30 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Wed 19-06-24 01:03:16, Shakeel Butt wrote:
> On Wed, Jun 19, 2024 at 09:19:41AM GMT, Michal Hocko wrote:
> > On Tue 18-06-24 14:34:21, 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.
> >
> > Long time ago, I've had a patch to do the once_per_callsite WARN. I
> > cannot find reference at the moment but it used stack depot to note
> > stacks that have already triggered. Back then there was no reponse on
> > the ML. Should I try to dig deep and recover it from my archives? I
> > think this is exactly kind of usecase where it would fit.
> >
>
> Do you mean something like warn once per unique call stack?
Exactly!
> If yes then
> I think that is better than the simple ratelimiting version as
> ratelimiting one may still miss some abusers and also may keep warning
> about the same abuser. Please do share your patch.
https://lore.kernel.org/all/20170103134424.28123-1-mhocko@kernel.org/
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 8:30 ` Michal Hocko
@ 2024-06-19 8:37 ` Shakeel Butt
2024-06-19 8:48 ` Michal Hocko
1 sibling, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2024-06-19 8:37 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Wed, Jun 19, 2024 at 10:30:46AM GMT, Michal Hocko wrote:
> On Wed 19-06-24 01:03:16, Shakeel Butt wrote:
> > On Wed, Jun 19, 2024 at 09:19:41AM GMT, Michal Hocko wrote:
> > > On Tue 18-06-24 14:34:21, 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.
> > >
> > > Long time ago, I've had a patch to do the once_per_callsite WARN. I
> > > cannot find reference at the moment but it used stack depot to note
> > > stacks that have already triggered. Back then there was no reponse on
> > > the ML. Should I try to dig deep and recover it from my archives? I
> > > think this is exactly kind of usecase where it would fit.
> > >
> >
> > Do you mean something like warn once per unique call stack?
>
> Exactly!
>
> > If yes then
> > I think that is better than the simple ratelimiting version as
> > ratelimiting one may still miss some abusers and also may keep warning
> > about the same abuser. Please do share your patch.
>
> https://lore.kernel.org/all/20170103134424.28123-1-mhocko@kernel.org/
Do you want to propose this patch again (after rebase to latest) or
should I take over?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 8:30 ` Michal Hocko
2024-06-19 8:37 ` Shakeel Butt
@ 2024-06-19 8:48 ` Michal Hocko
2024-06-19 17:47 ` Shakeel Butt
1 sibling, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2024-06-19 8:48 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Wed 19-06-24 10:30:46, Michal Hocko wrote:
> On Wed 19-06-24 01:03:16, Shakeel Butt wrote:
> > On Wed, Jun 19, 2024 at 09:19:41AM GMT, Michal Hocko wrote:
> > > On Tue 18-06-24 14:34:21, 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.
> > >
> > > Long time ago, I've had a patch to do the once_per_callsite WARN. I
> > > cannot find reference at the moment but it used stack depot to note
> > > stacks that have already triggered. Back then there was no reponse on
> > > the ML. Should I try to dig deep and recover it from my archives? I
> > > think this is exactly kind of usecase where it would fit.
> > >
> >
> > Do you mean something like warn once per unique call stack?
>
> Exactly!
>
> > If yes then
> > I think that is better than the simple ratelimiting version as
> > ratelimiting one may still miss some abusers and also may keep warning
> > about the same abuser. Please do share your patch.
>
> https://lore.kernel.org/all/20170103134424.28123-1-mhocko@kernel.org/
Btw. the code has changed a lot since 2017 when this was posted so it
will likely need a lot of massaging to rebase. Also I am not entirely
sure it is ok to change WARN_ONCE semantic like that anymore. Maybe we
need an explicit variant that does this per-call-site warnings. It is a
notable difference between library functions which can be called from
different callpaths and those that are used from a single place. I do
not have much time to dig deeper into this but if you want to take over
then go ahead. I still think this is a useful WARN_ONCE or in general
do_something_once semantic.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 8:48 ` Michal Hocko
@ 2024-06-19 17:47 ` Shakeel Butt
2024-06-19 19:30 ` Linus Torvalds
0 siblings, 1 reply; 14+ messages in thread
From: Shakeel Butt @ 2024-06-19 17:47 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Linus Torvalds, kernel-team, linux-mm,
linux-kernel, Kyle McMartin
On Wed, Jun 19, 2024 at 10:48:07AM +0200, Michal Hocko wrote:
> On Wed 19-06-24 10:30:46, Michal Hocko wrote:
> > On Wed 19-06-24 01:03:16, Shakeel Butt wrote:
> > > On Wed, Jun 19, 2024 at 09:19:41AM GMT, Michal Hocko wrote:
> > > > On Tue 18-06-24 14:34:21, 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.
> > > >
> > > > Long time ago, I've had a patch to do the once_per_callsite WARN. I
> > > > cannot find reference at the moment but it used stack depot to note
> > > > stacks that have already triggered. Back then there was no reponse on
> > > > the ML. Should I try to dig deep and recover it from my archives? I
> > > > think this is exactly kind of usecase where it would fit.
> > > >
> > >
> > > Do you mean something like warn once per unique call stack?
> >
> > Exactly!
> >
> > > If yes then
> > > I think that is better than the simple ratelimiting version as
> > > ratelimiting one may still miss some abusers and also may keep warning
> > > about the same abuser. Please do share your patch.
> >
> > https://lore.kernel.org/all/20170103134424.28123-1-mhocko@kernel.org/
>
> Btw. the code has changed a lot since 2017 when this was posted so it
> will likely need a lot of massaging to rebase. Also I am not entirely
> sure it is ok to change WARN_ONCE semantic like that anymore. Maybe we
> need an explicit variant that does this per-call-site warnings. It is a
> notable difference between library functions which can be called from
> different callpaths and those that are used from a single place. I do
> not have much time to dig deeper into this but if you want to take over
> then go ahead. I still think this is a useful WARN_ONCE or in general
> do_something_once semantic.
I think a separate variant like WARN_UNIQUE() would be better. I will
look into this.
Linus, please let me know if you have any concerns on the approach
Michal is suggesting i.e. a variant for warn once for unique call stack.
Shakeel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 17:47 ` Shakeel Butt
@ 2024-06-19 19:30 ` Linus Torvalds
2024-06-19 19:54 ` Michal Hocko
0 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2024-06-19 19:30 UTC (permalink / raw)
To: Shakeel Butt
Cc: Michal Hocko, Andrew Morton, kernel-team, linux-mm, linux-kernel,
Kyle McMartin
On Wed, 19 Jun 2024 at 10:47, Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> Linus, please let me know if you have any concerns on the approach
> Michal is suggesting i.e. a variant for warn once for unique call stack.
I think we should just try to change the existing WARN_ONCE(), and see
if it causes any issues.
A new "WARN_UNIQUE()" might be the borign and safe approach, but
(a) it won't actually be unique if you don't have stackdepot anyway,
and will just be WARN_ONCE
(b) I suspect most WARN_ONCE users really do want WARN_UNIQUE
so let's at least _start_ with just changing semantics of the existing
"once", and then if it causes problems we'll have to revisit this.
I doubt it will cause problems,
Linus
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 19:30 ` Linus Torvalds
@ 2024-06-19 19:54 ` Michal Hocko
0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2024-06-19 19:54 UTC (permalink / raw)
To: Linus Torvalds
Cc: Shakeel Butt, Andrew Morton, kernel-team, linux-mm, linux-kernel,
Kyle McMartin
On Wed 19-06-24 12:30:42, Linus Torvalds wrote:
> On Wed, 19 Jun 2024 at 10:47, Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > Linus, please let me know if you have any concerns on the approach
> > Michal is suggesting i.e. a variant for warn once for unique call stack.
>
> I think we should just try to change the existing WARN_ONCE(), and see
> if it causes any issues.
>
> A new "WARN_UNIQUE()" might be the borign and safe approach, but
>
> (a) it won't actually be unique if you don't have stackdepot anyway,
> and will just be WARN_ONCE
>
> (b) I suspect most WARN_ONCE users really do want WARN_UNIQUE
>
> so let's at least _start_ with just changing semantics of the existing
> "once", and then if it causes problems we'll have to revisit this.
>
> I doubt it will cause problems,
I would be careful about the WARN_ONCE used from stackdepot itself. It's
been some time since I have looked into that code but a quick grep tells
there is some usage.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-18 21:34 [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once Shakeel Butt
2024-06-18 21:40 ` Linus Torvalds
2024-06-19 7:19 ` Michal Hocko
@ 2024-06-19 12:49 ` Mateusz Guzik
2024-06-19 12:51 ` Mateusz Guzik
2024-06-19 17:52 ` Shakeel Butt
2 siblings, 2 replies; 14+ messages in thread
From: Mateusz Guzik @ 2024-06-19 12:49 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Michal Hocko, Linus Torvalds, kernel-team,
linux-mm, linux-kernel, Kyle McMartin
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@infradead.org>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 12:49 ` Mateusz Guzik
@ 2024-06-19 12:51 ` Mateusz Guzik
2024-06-19 17:52 ` Shakeel Butt
1 sibling, 0 replies; 14+ messages in thread
From: Mateusz Guzik @ 2024-06-19 12:51 UTC (permalink / raw)
To: Shakeel Butt
Cc: Andrew Morton, Michal Hocko, Linus Torvalds, kernel-team,
linux-mm, linux-kernel, Kyle McMartin
On Wed, Jun 19, 2024 at 2:49 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> 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@infradead.org>
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.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
I should clarify this is allocs of 104 bytes, not some outlandish size.
--
Mateusz Guzik <mjguzik gmail.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] mm: ratelimit oversized kvmalloc warnings instead of once
2024-06-19 12:49 ` Mateusz Guzik
2024-06-19 12:51 ` Mateusz Guzik
@ 2024-06-19 17:52 ` Shakeel Butt
1 sibling, 0 replies; 14+ messages in thread
From: Shakeel Butt @ 2024-06-19 17:52 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Andrew Morton, Michal Hocko, Linus Torvalds, kernel-team,
linux-mm, linux-kernel, Kyle McMartin
On Wed, Jun 19, 2024 at 02:49:21PM +0200, Mateusz Guzik wrote:
> 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@infradead.org>
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.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.
>
Thanks for the response and suggestion. I am inclined towards warn once
for each unique stack trace as suggested by Michal because I think it
would be useful in general.
^ permalink raw reply [flat|nested] 14+ messages in thread