Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1469126 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/5] arm64: Add uprobe support Pratyush Anand <panand@redhat.com> - 2016-08-24 09:20 +0200
    Re: [PATCH 5/5] arm64: Add uprobe support Will Deacon <will.deacon@arm.com> - 2016-08-24 18:00 +0200
    Re: [PATCH 5/5] arm64: Add uprobe support Oleg Nesterov <oleg@redhat.com> - 2016-08-24 18:00 +0200

#1469126 — Re: [PATCH 5/5] arm64: Add uprobe support

Hi Oleg,

Thanks a lot for your review, and sorry for delayed response.

On 09/08/2016:08:49:44 PM, Oleg Nesterov wrote:
> On 08/02, Pratyush Anand wrote:
> >
> > This patch adds support for uprobe on ARM64 architecture.
> 
> I know nothing about ARM, so I can't actually review this change.
> But it looks good to me ;)
> 
> Just one note,
> 
> > +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> > +{
> > +	struct uprobe_task *utask = current->utask;
> > +
> > +	/* saved fault code is restored in post_xol */
> > +	utask->autask.saved_fault_code = current->thread.fault_code;
> > +
> > +	/* An invalid fault code between pre/post xol event */
> > +	current->thread.fault_code = UPROBE_INV_FAULT_CODE;
> > +
> > +	/* Instruction point to execute ol */
> > +	instruction_pointer_set(regs, utask->xol_vaddr);
> > +
> > +	user_enable_single_step(current);
> 
> I don't think we want user_{enable,disable{_single_step in the long term,
> please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> the same.

IIUC, then you mean that TIF_SINGLESTEP is a per task flag, while
arch_uprobe_pre/post_xol() should enable/disable single stepping using a per
uprobe_task, and we should have a flag in "struct arch_uprobe_task" to handle
this, right?

> 
> However, I agree we can do this later and initial version can use these
> ptrace helpers.

Yes, I would also like to do that change latter, because these set of patches
have already been tested heavily with systemtap, so it would be better to go
with an incremental changes latter on.

~Pratyush

[toc] | [next] | [standalone]

#1469554

On Wed, Aug 24, 2016 at 05:47:11PM +0200, Oleg Nesterov wrote:
> On 08/24, Pratyush Anand wrote:
> >
> > > I don't think we want user_{enable,disable{_single_step in the long term,
> > > please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> > > /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> > > TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> > > the same.
> >
> > IIUC, then you mean that TIF_SINGLESTEP is a per task flag,
> 
> Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse
> each other. And uprobes simply doesn't need to set/clear it.

We're already using it for kprobes, hw_breakpoint and kgdb as well as
ptrace, so I'd rather uprobes either followed existing practice, or we
converted everybody off the current code.

In what way do things get confused?

> > while
> > arch_uprobe_pre/post_xol() should enable/disable single stepping using a per
> > uprobe_task,
> 
> I can't really answer since I know nothing about arm. x86 just needs to set
> X86_EFLAGS_TF, I guess arm needs to modify some register too?

We have {user,kernel}_{enable,disable}_single_step for managing the various
registers controlling the single-step state machine on arm64.

Will

[toc] | [prev] | [next] | [standalone]

#1469555

Hi Pratyush,

On 08/24, Pratyush Anand wrote:
>
> > I don't think we want user_{enable,disable{_single_step in the long term,
> > please look at 9bd1190a11c9d2 "uprobes/x86: Do not (ab)use TIF_SINGLESTEP
> > /user_*_single_step() for single-stepping". it seems that ARM64 sets/clears
> > TIF_SINGLESTEP. You can also lool at saved_tf logic, probably ARM64 needs
> > the same.
>
> IIUC, then you mean that TIF_SINGLESTEP is a per task flag,

Yes, and nobody but ptrace should use it, otherwise ptrace/uprobes can confuse
each other. And uprobes simply doesn't need to set/clear it.

> while
> arch_uprobe_pre/post_xol() should enable/disable single stepping using a per
> uprobe_task,

I can't really answer since I know nothing about arm. x86 just needs to set
X86_EFLAGS_TF, I guess arm needs to modify some register too?

> and we should have a flag in "struct arch_uprobe_task" to handle
> this, right?

Probably yes, because we need to record/restore X86_EFLAGS_TF in case it
was already set by ptrace or something else.

> > However, I agree we can do this later and initial version can use these
> > ptrace helpers.
>
> Yes, I would also like to do that change latter, because these set of patches
> have already been tested heavily with systemtap, so it would be better to go
> with an incremental changes latter on.

Yes, yes, I agree. Let me repeat that this patch looks good to me as initial
version, but obviously I can't really revit it and/or ack.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web