linux-kernel - Re: [PATCH 2/2] trace/kprobe: Remove limit on kretprobe maxactive

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: <20210616094622.c8bd37840898c67dddde1053@kernel.org>
Date:   Wed, 16 Jun 2021 09:46:22 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     Anton Blanchard <anton@...abs.org>, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH 2/2] trace/kprobe: Remove limit on kretprobe maxactive

On Tue, 15 Jun 2021 23:11:27 +0530
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:

> Masami Hiramatsu wrote:
> > On Mon, 14 Jun 2021 23:33:29 +0530
> > "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:
> > 
> >> We currently limit maxactive for a kretprobe to 4096 when registering
> >> the same through tracefs. The comment indicates that this is done so as
> >> to keep list traversal reasonable. However, we don't ever iterate over
> >> all kretprobe_instance structures. The core kprobes infrastructure also
> >> imposes no such limitation.
> >> 
> >> Remove the limit from the tracefs interface. This limit is easy to hit
> >> on large cpu machines when tracing functions that can sleep.
> >> 
> >> Reported-by: Anton Blanchard <anton@...abs.org>
> >> Signed-off-by: Naveen N. Rao <naveen.n.rao@...ux.vnet.ibm.com>
> > 
> > OK, but I don't like to just remove the limit (since it can cause
> > memory shortage easily.)
> > Can't we make it configurable? I don't mean Kconfig, but 
> > tracefs/options/kretprobe_maxactive, or kprobes's debugfs knob.
> > 
> > Hmm, maybe debugfs/kprobes/kretprobe_maxactive will be better since
> > it can limit both trace_kprobe and kprobes itself.
> 
> I don't think it is good to put a new tunable in debugfs -- we don't 
> have any kprobes tunable there, so this adds a dependency on debugfs 
> which shouldn't be necessary.
> 
> /proc/sys/debug/ may be a better fit since we have the 
> kprobes-optimization flag to disable optprobes there, though I'm not 
> sure if a new sysfs file is agreeable.

Indeed.

> But, I'm not too sure this really is a problem. Maxactive is a user 
> _opt-in_ feature which needs to be explicitly added to an event 
> definition. In that sense, isn't this already a tunable?

Let me explain the background of the limiation.

Maxactive is currently no limit for the kprobe kernel module API,
because the kernel module developer must take care of the max memory
usage (and they can).

But the tracefs user may NOT have enough information about what
happens if they pass something like 10M for maxactive (it will consume
around 500MB kernel memory for one kretprobe).

To avoid such trouble, I had set the 4096 limitation for the maxactive
parameter. Of course 4096 may not enough for some use-cases. I'm welcome
to expand it (e.g. 32k, isn't it enough?), but removing the limitation
may cause OOM trouble easily.

Thank you,

> 
> 
> - Naveen
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>