Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] Double free of `extra_consumers` during `fork()` due to missing pointer nullification in `dup_return_instance()` / `ri_pool_push()`. -- --- Patch [1]: [PATCH] uprobes: Free utask on dup_return_instance() failure --- commit 52bd4caeb5b9686b25fe707609034e9397c6100a Author: Keke Ming <[email protected]> uprobes: Free utask on dup_return_instance() failure dup_utask() installs the new uprobe_task in t->utask before copying return_instances. If dup_return_instance() fails, the partially copied utask is left attached to the child task. This patch frees the partially copied utask before returning -ENOMEM. > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 7709ea8824778..7547929ab294d 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -2140,8 +2140,10 @@ static int dup_utask(struct task_struct *t, struct > uprobe_task *o_utask) > p = &n_utask->return_instances; > for (o = o_utask->return_instances; o; o = o->next) { > n = dup_return_instance(o); > - if (!n) > + if (!n) { > + uprobe_free_utask(t); [Severity: Critical] This is a pre-existing issue, but can dup_return_instance() leave a dangling pointer that results in a double free of extra_consumers when uprobe_free_utask() is called here? If a multi-consumer uretprobe is hit, extra_consumers is allocated. When this returns, it is pushed to the reuse pool: kernel/events/uprobes.c:ri_pool_push() { ri->cons_cnt = 0; ri->next = utask->ri_pool; utask->ri_pool = ri; } This resets cons_cnt to 0 but retains the extra_consumers heap pointer. If the thread then hits a single-consumer uretprobe, alloc_return_instance() reuses the pooled object. push_consumer() increments cons_cnt to 1 and uses the inline consumer, leaving the old extra_consumers pointer untouched. When the task calls fork(), dup_return_instance() does a shallow copy: kernel/events/uprobes.c:dup_return_instance() { ri = kmemdup(old, sizeof(*ri), GFP_KERNEL); if (!ri) return NULL; if (unlikely(old->cons_cnt > 1)) { ri->extra_consumers = kmemdup(old->extra_consumers, ... ... } Since old->cons_cnt == 1, it skips deep-copying or clearing extra_consumers. Both the parent and child tasks now share the exact same heap pointer. When the tasks exit, or if dup_return_instance() fails and triggers the new cleanup path in this patch, ri_free() will execute kfree(ri->extra_consumers) in both contexts, freeing the same memory twice. > return -ENOMEM; > + } > > /* if uprobe is non-NULL, we'll have an extra refcount for > uprobe */ > uprobe = hprobe_expire(&o->hprobe, true); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
