Re: [PATCH v4 1/1] arm64: ptrace: fix hw_break_set() to set addr and ctrl together

[Bill Tsui <b10902118@ntu.edu.tw>]

On 2025-11-15 00:14, Will Deacon wrote:
> ... but you only implement this for the native (64-bit) case, so I 
> don't
> understand how it helps with the problem above.

Yes it is only for 64-bit tracer to debug 32-bit processes because
64-bit tracer always use 64-bit ptrace API (PTRACE_SETREGSET) regardless
of tracee, so only 64-bit code were changed.

>> @@ -524,9 +506,6 @@ static int hw_break_set(struct task_struct 
>> *target,
>>  			return -EINVAL;
>>  		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
>>  					 offset, offset + PTRACE_HBP_ADDR_SZ);
>> -		if (ret)
>> -			return ret;
>> -		ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
>>  		if (ret)
>>  			return ret;
>>  		offset += PTRACE_HBP_ADDR_SZ;
>> @@ -537,10 +516,11 @@ static int hw_break_set(struct task_struct 
>> *target,
>>  					 offset, offset + PTRACE_HBP_CTRL_SZ);
>>  		if (ret)
>>  			return ret;
>> -		ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
>> +		offset += PTRACE_HBP_CTRL_SZ;
>> +
>> +		ret = ptrace_hbp_set(note_type, target, idx, addr, ctrl);
> 
> Doesn't this break the case where userspace tries only to set the 
> address?
> The loop will break out when !count without updating anything.
> 

The case doesn't exist for normal use. I saw that as a validation
rather than an intended stop point. If in doubt, I can remove it.

The signature from man ptrace(2)
	ptrace(PTRACE_SETREGSET, pid, NT_foo, &iov)
requires all registers' values to be specified in iov in order. And the
unit of debug registers is the inline structure in user_hwdebug_state:
	struct {
		__u64	addr;
		__u32	ctrl;
		__u32	pad;
	}		dbg_regs[16];
and userspace uses by:
	struct user_hwdebug_state dreg_state;
	ioVec.iov_base = &dreg_state;
	ioVec.iov_len = sizeof(dreg_state.dbg_info) + sizeof(dreg_state.pad) 
+				(sizeof(dreg_state.dbg_regs[0]) * m_max_hwp_supported);

	for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
		dreg_state.dbg_regs[i].addr = m_hwp_regs[i].address;
		dreg_state.dbg_regs[i].ctrl = m_hwp_regs[i].control;
	}
The only case of setting only address is to have iov length only reach
the first register's .addr, but I don't think that memory layout should
be relied on.

> As I mentioned before, I'd prefer to leave this code as-is short of
> removing the indirection through perf entirely.

I am working on that, the side effects are hard to trace, and I think
this patch can make removing perf easier by reducing calls to it and
make clearer what hardware API ptrace needs. This recurs to me and I
think it can be a separated patch.

Here is my progress on removing perf:
I tried to directly set by hw_breakpoint_control, which perf finally
use, but we also relied on perf event handling cpu interrupts when
breakpoints get triggered. This is the hardest part and I am learning
how the kernel handles it across so many others. I feel that the use
of perf has its reasons but definitely there is space to improve.

Anyway, I respect maintainer's decision. I just felt that you thought
this is complicated so I simplifed it. Now I explains the correctness
and how it relates to your final goal. If there are still concerns,
please directly reject me, then I can also stop holding on it.

Thanks,
Bill