Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758597Ab3CYPwr (ORCPT ); Mon, 25 Mar 2013 11:52:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4530 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758534Ab3CYPwq (ORCPT ); Mon, 25 Mar 2013 11:52:46 -0400 Date: Mon, 25 Mar 2013 16:51:55 +0100 From: Anton Arapov To: Oleg Nesterov 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: <20130325155155.GB2178@bandura.brq.redhat.com> References: <1363957745-6657-1-git-send-email-anton@redhat.com> <1363957745-6657-5-git-send-email-anton@redhat.com> <20130324152651.GC17037@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130324152651.GC17037@redhat.com> X-PGP-Key: http://people.redhat.com/aarapov/gpg User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3648 Lines: 139 On Sun, Mar 24, 2013 at 04:26:51PM +0100, Oleg Nesterov wrote: > 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; I will do the appropriate changes. Thanks! Anton. > 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/