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
