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

[Jiri Olsa]

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