linux-kernel - Re: [PATCH] kretprobe events missing on 2-core KVM guest

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: <20221107133628.GA31649@openwall.com>
Date:   Mon, 7 Nov 2022 14:36:28 +0100
From:   Solar Designer <solar@...nwall.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     wuqiang <wuqiang.matt@...edance.com>, davem@...emloft.net,
        anil.s.keshavamurthy@...el.com, naveen.n.rao@...ux.ibm.com,
        linux-kernel@...r.kernel.org, mattwu@....com,
        Adam Zabrocki <pi3@....com.pl>
Subject: Re: [PATCH] kretprobe events missing on 2-core KVM guest

On Wed, Oct 26, 2022 at 12:33:15AM +0900, Masami Hiramatsu wrote:
> On Tue, 25 Oct 2022 18:01:17 +0800
> wuqiang <wuqiang.matt@...edance.com> wrote:
> 
> > Default value of maxactive is set as num_possible_cpus() for nonpreemptable
> > systems. For a 2-core system, only 2 kretprobe instances would be allocated
> > in default, then these 2 instances for execve kretprobe are very likely to
> > be used up with a pipelined command.
> > 
> > This patch increases the minimum of maxactive to 10.
> 
> This looks reasonable to me.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>

Reasonable yes, but:

Is 10 enough?  How exactly do those instances get used up without
preemption?  Perhaps because execve can sleep?  If so, perhaps we should
use the same logic without preemption that we do with preemption?  So
maybe just make this line unconditional? -

		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());

Also, the behavior was documented in Documentation/trace/kprobes.rst, so
perhaps that file should be updated at the same time with the code.

> > Signed-off-by: wuqiang <wuqiang.matt@...edance.com>
> > ---
> >  kernel/kprobes.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index 3220b0a2fb4a..b781dee3f552 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2211,7 +2211,7 @@ int register_kretprobe(struct kretprobe *rp)
> >  #ifdef CONFIG_PREEMPTION
> >  		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
> >  #else
> > -		rp->maxactive = num_possible_cpus();
> > +		rp->maxactive = max_t(unsigned int, 10, num_possible_cpus());
> >  #endif
> >  	}
> >  #ifdef CONFIG_KRETPROBE_ON_RETHOOK
> > -- 
> > 2.34.1
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Thanks,

Alexander