Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754021Ab3CXP3U (ORCPT ); Sun, 24 Mar 2013 11:29:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4025 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752781Ab3CXP3T (ORCPT ); Sun, 24 Mar 2013 11:29:19 -0400 Date: Sun, 24 Mar 2013 16:26:51 +0100 From: Oleg Nesterov To: Anton Arapov Cc: Srikar Dronamraju , LKML , Josh Stone , Frank Eigler , Peter Zijlstra , Ingo Molnar , Ananth N Mavinakayanahalli , adrian.m.negreanu@intel.com, Torsten.Polle@gmx.de Subject: Re: [PATCH 4/7] uretprobes: return probe entry, prepare_uretprobe() Message-ID: <20130324152651.GC17037@redhat.com> References: <1363957745-6657-1-git-send-email-anton@redhat.com> <1363957745-6657-5-git-send-email-anton@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1363957745-6657-5-git-send-email-anton@redhat.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3278 Lines: 133 On 03/22, Anton Arapov wrote: > > void uprobe_free_utask(struct task_struct *t) > { > struct uprobe_task *utask = t->utask; > + struct return_instance *ri, *tmp; > > if (!utask) > return; > @@ -1325,6 +1334,15 @@ void uprobe_free_utask(struct task_struct *t) > if (utask->active_uprobe) > put_uprobe(utask->active_uprobe); > > + ri = utask->return_instances; You also need to nullify ->return_instances before return, otherwise it can be use-after-freed later. uprobe_free_utask() can also be called when the task execs. > + while (ri) { > + put_uprobe(ri->uprobe); > + > + tmp = ri; > + ri = ri->next; > + kfree(tmp); > + } This is really minor, but I can't resist. Both put_uprobe() and kfree() work with the same object, it would be more clean to use the same var. Say, while (ri) { tmp = ri; ri = ri->next; put_uprobe(tmp->uprobe); kfree(tmp); } > +static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs) > +{ ... > + > + prev_ret_vaddr = -1; > + if (utask->return_instances) > + prev_ret_vaddr = utask->return_instances->orig_ret_vaddr; > + > + ri = kzalloc(sizeof(struct return_instance), GFP_KERNEL); > + if (!ri) > + return; > + > + ri->dirty = false; > + trampoline_vaddr = get_trampoline_vaddr(area); > + ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs); > + > + /* > + * We don't want to keep trampoline address in stack, rather keep the > + * original return address of first caller thru all the consequent > + * instances. This also makes breakpoint unwrapping easier. > + */ > + if (ret_vaddr == trampoline_vaddr) { > + if (likely(prev_ret_vaddr != -1)) { > + ri->dirty = true; > + ret_vaddr = prev_ret_vaddr; > + } else { > + /* > + * This situation is not possible. Likely we have an > + * attack from user-space. Die. > + */ > + printk(KERN_ERR "uprobe: something went wrong " > + "pid/tgid=%d/%d", current->pid, current->tgid); > + send_sig(SIGSEGV, current, 0); > + kfree(ri); > + return; > + } > + } > + > + if (likely(ret_vaddr != -1)) { > + atomic_inc(&uprobe->ref); > + ri->uprobe = uprobe; > + ri->orig_ret_vaddr = ret_vaddr; > + > + /* add instance to the stack */ > + ri->next = utask->return_instances; > + utask->return_instances = ri; > + > + return; > + } > + > + kfree(ri); > +} Anton, this really doesn't look clear/clean. Why do you need prev_ret_vaddr in advance? Why do you need it at all? why do you delay the "ret_vaddr == -1" errorcheck? And ->dirty looks confusing... perhaps ->chained ? ri = kzalloc(...); if (!ri) return; ret_vaddr = arch_uretprobe_hijack_return_addr(...); if (ret_vaddr == -1) goto err; if (ret_vaddr == trampoline_vaddr) { if (!utask->return_instances) { // This situation is not possible. // (not sure we should send SIGSEGV) pr_warn(...); goto err; } ri->chained = true; ret_vaddr = utask->return_instances->orig_ret_vaddr; } fill-ri-and-add-push-it; return; err: kfree(ri); return; Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/