Re: [REVIEW][PATCH 02/20] signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap

[Eric W. Biederman]

Christoph Hellwig <hch@xxxxxxxxxxxxx> writes:

>>  
>>  	clear_siginfo(&info);
>> -	fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
>> +	tsk->thread.trap_nr = X86_TRAP_DB;
>> +	tsk->thread.error_code = error_code;
>> +
>> +	info.si_signo = SIGTRAP;
>> +	info.si_code = si_code;
>> +	info.si_addr = user_mode(regs) ? (void __user *)regs->ip : NULL;
>
> clear_siginfo already zeroes the whole structure, so this could be
> written more clearly as:
>
> 	if (user_mode(regs)
> 		info.si_addr = (void __user *)regs->ip;

That change does not make sense in this particular patch as it is just
code motion.

It also does not make sense in the final version of the code at
the end of the patch series which is:

void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
					 int error_code, int si_code)
{
	tsk->thread.trap_nr = X86_TRAP_DB;
	tsk->thread.error_code = error_code;

	/* Send us the fake SIGTRAP */
	force_sig_fault(SIGTRAP, si_code,
			user_mode(regs) ? (void __user *)regs->ip : NULL, tsk);
}

In this version the test also makes sense because struct siginfo is
gone because manually filling it out results in more bugs than
necessary.  That is now left up to force_sig_fault.

I was hoping that we could show that user_mode(regs) is always true.
But according to arch/x86/kernel/traps.c:do_debug watch points that will
trigger even when the kernel accesses user space addresses.

Eric