Re: [PATCH v5 7/14] x86 support for Uprobes

Christoph Hellwig
2010-06-14T17:54:49Z

On Mon, Jun 14, 2010 at 01:59:13PM +0530, Srikar Dronamraju wrote:
> @@ -850,7 +850,19 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
>
> if (thread_info_flags & _TIF_UPROBE) {
> clear_thread_flag(TIF_UPROBE);
> +#ifdef CONFIG_X86_32
> + /*
> + * On x86_32, do_notify_resume() gets called with
> + * interrupts disabled. Hence enable interrupts if they
> + * are still disabled.
> + */
> + native_irq_enable();
> +#endif
> uprobe_notify_resume(regs);
> +
> +#ifdef CONFIG_X86_32
> + native_irq_disable();
> +#endif

I'm no x86 port guru, but this looks rather worriesome to me. Why does
do_notify_resume have different calling conventions on 32 vs 64-bit?
And if there is a good reason that 32-bit has them disabled, why is
enabling them in the middle of do_notify_resume okay?

> +void arch_uprobe_disable_sstep(struct pt_regs *regs)
> +{
> + /* Disable single-stepping by clearing what we set */
> + clear_thread_flag(TIF_SINGLESTEP);
> + clear_thread_flag(TIF_FORCED_TF);
> + regs->flags &= ~X86_EFLAGS_TF;
> +}

This seems to have one layer of indentation too much.