diff mbox series

[RFC,1/5] powerpc: Fix inverted active predicate for setting the EBB regset

Message ID 20180607152534.29427-2-pedromfc@linux.vnet.ibm.com (mailing list archive)
State RFC
Headers show
Series powerpc: Misc. ptrace regset fixes | expand

Commit Message

Pedro Franco de Carvalho June 7, 2018, 3:25 p.m. UTC 
Currently, the ebb_set function for writing to the EBB regset returns
ENODATA when ebb is active in the thread, and copies in the data when
it is inactive. This patch inverts the condition so that it matches
ebb_get and ebb_active.
---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Ellerman June 13, 2018, 2:15 a.m. UTC | #1 
Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com> writes:

> Currently, the ebb_set function for writing to the EBB regset returns
> ENODATA when ebb is active in the thread, and copies in the data when
> it is inactive. This patch inverts the condition so that it matches
> ebb_get and ebb_active.
> ---
>  arch/powerpc/kernel/ptrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Hi Pedro,

Thanks for looking into this, how did you detect this? Do you have a
test case?

I don't think Anshuman wrote it this way on purpose, but added him to Cc
in case he remembers.

But I don't think this fix is necessarily right. If we are setting the
EBB regs via ptrace then it doesn't matter if they were previously in
use or not, we should just set them. What *does* matter is that at the
end of the function we set used_ebb to true, because otherwise the
values we have set will not actually be used when the process is
rescheduled.

cheers

> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index d23cf632edf0..6618570c6d56 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1701,7 +1701,7 @@ static int ebb_set(struct task_struct *target,
>  	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
>  		return -ENODEV;
>  
> -	if (target->thread.used_ebb)
> +	if (!target->thread.used_ebb)
>  		return -ENODATA;
>  
>  	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> -- 
> 2.13.6
Michael Ellerman June 13, 2018, 4:09 a.m. UTC | #2 
Michael Ellerman <mpe@ellerman.id.au> writes:

> Pedro Franco de Carvalho <pedromfc@linux.vnet.ibm.com> writes:
>
>> Currently, the ebb_set function for writing to the EBB regset returns
>> ENODATA when ebb is active in the thread, and copies in the data when
>> it is inactive. This patch inverts the condition so that it matches
>> ebb_get and ebb_active.
>> ---
>>  arch/powerpc/kernel/ptrace.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Hi Pedro,
>
> Thanks for looking into this, how did you detect this? Do you have a
> test case?

I thought we had a test case for this, and it seems that we did, but it
never made it into mainline.

Anshuman wrote it originally but then passed it onto Simon who
resubmitted it and I merged it, but then I must have reverted the EBB
test because it was failing, see:

  http://patchwork.ozlabs.org/patch/676833/


So it would be good to get that test case working reliably. I'm not sure
if would have found this bug anyway, but it would be a start.

cheers
Pedro Franco de Carvalho June 14, 2018, 1:52 p.m. UTC | #3 
Michael Ellerman <mpe@ellerman.id.au> writes:

> Hi Pedro,
>
> Thanks for looking into this, how did you detect this? Do you have a
> test case?

Hello,

I'm working on allowing these registers to be accessed through GDB,
which is where I saw this happen. Then I used a small program that
traces another, then reads and tries to write to the regset, but not in
linux selftest format.

> I don't think Anshuman wrote it this way on purpose, but added him to Cc
> in case he remembers.
>
> But I don't think this fix is necessarily right. If we are setting the
> EBB regs via ptrace then it doesn't matter if they were previously in
> use or not, we should just set them. What *does* matter is that at the
> end of the function we set used_ebb to true, because otherwise the
> values we have set will not actually be used when the process is
> rescheduled.

Now I'm not sure why the ptrace calls for the ebb regset are guarded
with used_ebb in the first place. The save/restore_sprs functions in
kernel/process.c seem to handle the ebb registers regardless of this
flag, and it seems to be possible for user programs to read and write to
these registers even without having first created a perf event.

The flag only appears to be used in perf/core_book3s.c in the
ebb_event_add function, and the pmu registers (mmcr0, etc) only seem to
be saved to and restored from the thread_struct through
ebb_switch_in/out. So maybe the original intent was to guard the
pmu_get/set functions with used_ebb instead?

I'm not sure about this, because I don't know if it possible for a
ptrace call to happen between ebb_event_add and the first ebb_switch_in
for a thread.

Thanks!

--
Pedro
diff mbox series

Patch

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index d23cf632edf0..6618570c6d56 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1701,7 +1701,7 @@  static int ebb_set(struct task_struct *target,
 	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
 		return -ENODEV;
 
-	if (target->thread.used_ebb)
+	if (!target->thread.used_ebb)
 		return -ENODATA;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,