Re: [PATCH 5/5] arm64: Add uprobe support

[Catalin Marinas <catalin.marinas@arm.com>]

On Wed, Sep 21, 2016 at 04:30:47PM +0530, Pratyush Anand wrote:
> On 20/09/2016:05:59:46 PM, Catalin Marinas wrote:
> > On Tue, Aug 02, 2016 at 11:00:09AM +0530, Pratyush Anand wrote:
> > > --- a/arch/arm64/include/asm/thread_info.h
> > > +++ b/arch/arm64/include/asm/thread_info.h
> > > @@ -109,6 +109,7 @@ static inline struct thread_info *current_thread_info(void)
> > >  #define TIF_NEED_RESCHED	1
> > >  #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
> > >  #define TIF_FOREIGN_FPSTATE	3	/* CPU's FP state is not current's */
> > > +#define TIF_UPROBE		5	/* uprobe breakpoint or singlestep */
> > 
> > Nitpick: you can just use 4 until we cover this gap.
> 
> Hummm..as stated in commit log, Shi Yang suggested to define TIF_UPROBE as 5 in
> stead of 4, since 4 has already been used in -rt kernel. May be, I can put a
> comment in code as well.
> Or, keep it 4 and -rt kernel will change their definitions. I am OK with both.
> let me know.

I forgot about the -rt kernel. I guess the -rt guys need to rebase the
patches anyway on top of mainline, so it's just a matter of sorting out
a minor conflict (as I already said, these bits are internal to the
kernel, so no ABI affected). For now, just use 4 so that we avoid
additional asm changes.

> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -688,7 +688,8 @@ ret_fast_syscall:
> > >  	ldr	x1, [tsk, #TI_FLAGS]		// re-check for syscall tracing
> > >  	and	x2, x1, #_TIF_SYSCALL_WORK
> > >  	cbnz	x2, ret_fast_syscall_trace
> > > -	and	x2, x1, #_TIF_WORK_MASK
> > > +	mov     x2, #_TIF_WORK_MASK
> > > +	and     x2, x1, x2
> > 
> > Is this needed because _TIF_WORK_MASK cannot work as an immediate value
> > to 'and'? We could reorder the TIF bits, they are not exposed to user to
> > have ABI implications.
> 
> _TIF_WORK_MASK is defined as follows:
> 
> #define _TIF_WORK_MASK          (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>                                   _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
>                                   _TIF_UPROBE)
> Re-ordering will not help, because 0-3 have already been used by previous
> definitions. Only way to use immediate value could be if, TIF_UPROBE is defined
> as 4. 

Yes, see above.

> > > +unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
> > > +{
> > > +	return instruction_pointer(regs);
> > > +}
> > > +
> > > +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> > > +		unsigned long addr)
> > > +{
> > > +	probe_opcode_t insn;
> > > +
> > > +	/* TODO: Currently we do not support AARCH32 instruction probing */
> > 
> > Is there a way to check (not necessarily in this file) that we don't
> > probe 32-bit tasks?
> 
> - Well, I do not have complete idea about it that, how it can be done. I think
>   we can not check that just by looking a single bit in an instruction.
>   My understanding is that, we can only know about it when we are executing the
>   instruction, by reading pstate, but that would not be useful for uprobe
>   instruction analysis.
>   
>   I hope, instruction encoding for aarch32 and aarch64 are different, and by
>   analyzing for all types of aarch32 instructions, we will be able to decide
>   that whether instruction is 32 bit trace-able or not.  Accordingly, we can use
>   either BRK or BKPT instruction for breakpoint generation.

We may have some unrelated instruction encoding overlapping but I
haven't checked. I was more thinking about whether we know which task is
being probed and check is_compat_task() or maybe using
compat_user_mode(regs).

-- 
Catalin