From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751886AbbEGRLx (ORCPT <rfc822;w@1wt.eu>);
	Thu, 7 May 2015 13:11:53 -0400
Received: from mx1.redhat.com ([209.132.183.28]:58434 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751434AbbEGRLt (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Thu, 7 May 2015 13:11:49 -0400
Date: Thu, 7 May 2015 19:11:19 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>,
        Anton Arapov <arapov@gmail.com>, David Long <dave.long@linaro.org>,
        Denys Vlasenko <dvlasenk@redhat.com>,
        "Frank Ch. Eigler" <fche@redhat.com>, Ingo Molnar <mingo@kernel.org>,
        Jan Willeke <willeke@de.ibm.com>, Jim Keniston <jkenisto@us.ibm.com>,
        Mark Wielaard <mjw@redhat.com>, Pratyush Anand <panand@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 07/10] uprobes/x86: Introduce arch_uretprobe_is_alive()
Message-ID: <20150507171119.GC18652@redhat.com>
References: <20150504124835.GA22462@redhat.com> <20150504124914.GA22512@redhat.com> <20150507110852.GF30396@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150507110852.GF30396@linux.vnet.ibm.com>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 05/07, Srikar Dronamraju wrote:
>
> >
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index f011fd0..60777f3 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -57,6 +57,7 @@ struct arch_uprobe {
> >  };
> >
> >  struct arch_uretprobe {
> > +	unsigned long	sp;
>
> While this looks good, I was wondering if you did think of having the sp
> in the return_instance structure itself. I mean can we use
> user_stack_pointer() to populate the ri->sp?

Yes, yes, I considered this option. And note that we can cleanup the
(a bit ugly) arch_uretprobe_hijack_return_addr() if we export
"struct return_instance" and pass it to arch_ helper.

> In which case the weak function itself should suffice for most archs.
>
> Something like this.
> prepare_uretprobe() we can have
> ri->sp = user_stack_pointer(regs)

Yes, and we can do this without changing arch_uretprobe_hijack_return_addr()
interface (which imo should be changed anyway, but this is off-topic).

> and handle_trampoline() would call something like
>
> 	arch_uretprobe_is_alive(next->sp, regs);
>
> bool __weak arch_uretprobe_is_alive(unsigned long sp, struct pt_regs *regs)
> {
> 	return user_stack_pointer(regs) <= sp;
> }

The problem is, I simply do not know if this is right on !x86.

And. I wanted to ensure that if (say) arch/ppc needs something else to
save/check in hijack/alive, then this architecture can just add the new
members in arch_uretprobe and change the arch_ helpers.

> Am I missing something?

I do not know. Lets wait for the comments from arch/ maintainers?

Oleg.