linux-kernel - Re: [PATCH v3] kretprobe: percpu support

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: <20200218205025.4047cf0506f56b18f9a989c4@kernel.org>
Date:   Tue, 18 Feb 2020 20:50:25 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Luigi Rizzo <lrizzo@...gle.com>
Cc:     linux-kernel@...r.kernel.org, naveen.n.rao@...ux.ibm.com,
        anil.s.keshavamurthy@...el.com, David Miller <davem@...emloft.net>,
        gregkh@...uxfoundation.org, Ingo Molnar <mingo@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v3] kretprobe: percpu support

On Tue, 18 Feb 2020 01:39:40 -0800
Luigi Rizzo <lrizzo@...gle.com> wrote:

> On Mon, Feb 17, 2020 at 11:55 PM Masami Hiramatsu <mhiramat@...nel.org> wrote:
> >
> > Hi Luigi,
> >
> > On Mon, 17 Feb 2020 16:56:59 -0800
> > Luigi Rizzo <lrizzo@...gle.com> wrote:
> >
> > > kretprobe uses a list protected by a single lock to allocate a
> > > kretprobe_instance in pre_handler_kretprobe(). This works poorly with
> > > concurrent calls.
> >
> > Yes, there are several potential performance issue and the recycle
> > instance is one of them. However, I think this spinlock is not so racy,
> > but noisy (especially on many core machine) right?
> 
> correct, it is especially painful on 2+ sockets and many-core systems
> when attaching kretprobes on otherwise uncontended paths.
> 
> >
> > Racy lock is the kretprobe_hash_lock(), I would like to replace it
> > with ftrace's per-task shadow stack. But that will be available
> > only if CONFIG_FUNCTION_GRAPH_TRACER=y (and instance has no own
> > payload).
> >
> > > This patch offers a simplified fix: the percpu_instance flag indicates
> > > that we allocate one instance per CPU, and the allocation is contention
> > > free, but we allow only have one pending entry per CPU (this could be
> > > extended to a small constant number without much trouble).
> >
> > OK, the percpu instance idea is good to me, and I think it should be
> > default option. Unless user specifies the number of instances, it should
> > choose percpu instance by default.
> 
> That was my initial implementation, which would not even need the
> percpu_instance
> flag in struct kretprobe. However, I felt that changing the default
> would have subtle
> side effects (e.g., only one outstanding call per CPU) so I thought it
> would be better
> to leave the default unchanged and make the flag explicit.
> 
> > Moreover, this makes things a bit complicated, can you add per-cpu
> > instance array? If it is there, we can remove the old recycle rp insn
> > code.
> 
> Can you clarify what you mean by "per-cpu instance array" ?
> Do you mean allowing multiple outstanding entries per cpu?

Yes, either allocating it on percpu area or allocating arraies
on percpu pointer is OK. e.g.

	instance_size = sizeof(*rp->pcpu) + rp->data_size;
	rp->pcpu = __alloc_percpu(instance_size * array_size,
				  __alignof__(*rp->pcpu));

And we will search free ri on the percpu array by checking ri->rp == NULL. 

Thank you,

> 
> I will address your code comments in an updated patch.
> 
> thanks
> luigi


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