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

[Christoph Hellwig]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/