linux-kernel - Re: [PATCHv3 1/7] uprobe: Add support for session consumer

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: <ZuQjPCdLkKnPQsu0@krava>
Date: Fri, 13 Sep 2024 13:34:20 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Jiri Olsa <olsajiri@...il.com>, Peter Zijlstra <peterz@...radead.org>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
	Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>,
	Yonghong Song <yhs@...com>,
	John Fastabend <john.fastabend@...il.com>,
	KP Singh <kpsingh@...omium.org>, Hao Luo <haoluo@...gle.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCHv3 1/7] uprobe: Add support for session consumer

On Fri, Sep 13, 2024 at 12:57:51PM +0200, Oleg Nesterov wrote:
> On 09/13, Jiri Olsa wrote:
> >
> > I'm not sure the realloc will help, I feel like we need to allocate return
> > consumer for each called handler separately to be safe
> 
> How about something like the (pseudo) code below? Note that this way
> we do not need uprobe->consumers_cnt. Note also that krealloc() should
> be unlikely and it checks ksize() before it does another allocation.
> 
> Oleg.
> 
> static size_t ri_size(int consumers_cnt)
> {
> 	return sizeof(struct return_instance) +
> 		      sizeof(struct return_consumer) * consumers_cnt;
> }
> 
> #define DEF_CNT	4	// arbitrary value
> 
> static struct return_instance *alloc_return_instance(void)
> {
> 	struct return_instance *ri;
> 
> 	ri = kzalloc(ri_size(DEF_CNT), GFP_KERNEL);
> 	if (!ri)
> 		return ZERO_SIZE_PTR;
> 
> 	ri->consumers_cnt = DEF_CNT;
> 	return ri;
> }
> 
> static struct return_instance *push_id_cookie(struct return_instance *ri, int idx,
> 						__u64 id, __u64 cookie)
> {
> 	if (unlikely(ri == ZERO_SIZE_PTR))
> 		return ri;
> 
> 	if (unlikely(idx >= ri->consumers_cnt)) {
> 		ri->consumers_cnt += DEF_CNT;
> 		ri = krealloc(ri, ri_size(ri->consumers_cnt), GFP_KERNEL);
> 		if (!ri) {
> 			kfree(ri);
> 			return ZERO_SIZE_PTR;
> 		}
> 	}
> 
> 	ri->consumers[idx].id = id;
> 	ri->consumers[idx].cookie = cookie;
> 	return ri;
> }
> 
> static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
> {
> 	...
> 	struct return_instance *ri = NULL;
> 	int push_idx = 0;
> 
> 	list_for_each_entry_rcu(uc, &uprobe->consumers, cons_node, rcu_read_lock_trace_held()) {
> 		__u64 cookie = 0;
> 		int rc = 0;
> 
> 		if (uc->handler)
> 			rc = uc->handler(uc, regs, &cookie);
> 
> 		remove &= rc;
> 		has_consumers = true;
> 
> 		if (!uc->ret_handler || rc == UPROBE_HANDLER_REMOVE || rc == 2)
> 			continue;
> 
> 		if (!ri)
> 			ri = alloc_return_instance();
> 
> 		// or, better if (rc = UPROBE_HANDLER_I_WANT_MY_COOKIE)
> 		if (uc->handler))
> 			ri = push_id_cookie(ri, push_idx++, uc->id, cookie);
> 	}
> 
> 	if (!ZERO_OR_NULL_PTR(ri)) {

should we rather bail out right after we fail to allocate ri above?

> 		ri->consumers_cnt = push_idx;
> 		prepare_uretprobe(uprobe, regs, ri);
> 	}
> 
> 	...
> }
> 

nice, I like that, will try to to plug it with the rest

thanks,
jirka