2020-09-22 09:16:54

by Zhenzhong Duan

[permalink] [raw]
Subject: [PATCH] csky: Fix a size determination in gpr_get()

"*" is missed in size determination as we are passing register set
rather than a pointer.

Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
Signed-off-by: Zhenzhong Duan <[email protected]>
---
arch/csky/kernel/ptrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
index d822144906ac..a4cf2e2ac15a 100644
--- a/arch/csky/kernel/ptrace.c
+++ b/arch/csky/kernel/ptrace.c
@@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
/* Abiv1 regs->tls is fake and we need sync here. */
regs->tls = task_thread_info(target)->tp_value;

- return membuf_write(&to, regs, sizeof(regs));
+ return membuf_write(&to, regs, sizeof(*regs));
}

static int gpr_set(struct task_struct *target,
--
2.25.1


2020-09-22 16:30:46

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> "*" is missed in size determination as we are passing register set
> rather than a pointer.

Ack. I can push it to Linus today, unless you want it to go through
csky tree. Preferences?

> Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
> Signed-off-by: Zhenzhong Duan <[email protected]>
> ---
> arch/csky/kernel/ptrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
> index d822144906ac..a4cf2e2ac15a 100644
> --- a/arch/csky/kernel/ptrace.c
> +++ b/arch/csky/kernel/ptrace.c
> @@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
> /* Abiv1 regs->tls is fake and we need sync here. */
> regs->tls = task_thread_info(target)->tp_value;
>
> - return membuf_write(&to, regs, sizeof(regs));
> + return membuf_write(&to, regs, sizeof(*regs));
> }
>
> static int gpr_set(struct task_struct *target,
> --
> 2.25.1
>

2020-09-23 01:25:32

by Guo Ren

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

Thx Duan,

Acked-by: Guo Ren <[email protected]>

Hi AI,

I found the broken commit still has a question:

> commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> Author: Al Viro <[email protected]>
> Date: Tue Jun 16 15:28:29 2020 -0400

> csky: switch to ->regset_get()

> NB: WTF "- what the fuck :(" is fpregs_get() playing at???
The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
fp regs are stored in threads' context.
So, WTF question for?

Best Regards
Guo Ren

On Wed, Sep 23, 2020 at 12:29 AM Al Viro <[email protected]> wrote:
>
> On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> > "*" is missed in size determination as we are passing register set
> > rather than a pointer.
>
> Ack. I can push it to Linus today, unless you want it to go through
> csky tree. Preferences?
>
> > Fixes: dcad7854fcce ("sky: switch to ->regset_get()")
> > Signed-off-by: Zhenzhong Duan <[email protected]>
> > ---
> > arch/csky/kernel/ptrace.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/csky/kernel/ptrace.c b/arch/csky/kernel/ptrace.c
> > index d822144906ac..a4cf2e2ac15a 100644
> > --- a/arch/csky/kernel/ptrace.c
> > +++ b/arch/csky/kernel/ptrace.c
> > @@ -83,7 +83,7 @@ static int gpr_get(struct task_struct *target,
> > /* Abiv1 regs->tls is fake and we need sync here. */
> > regs->tls = task_thread_info(target)->tp_value;
> >
> > - return membuf_write(&to, regs, sizeof(regs));
> > + return membuf_write(&to, regs, sizeof(*regs));
> > }
> >
> > static int gpr_set(struct task_struct *target,
> > --
> > 2.25.1
> >



--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-09-23 01:26:14

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 08:03:20AM +0800, Guo Ren wrote:
> Thx Duan,
>
> Acked-by: Guo Ren <[email protected]>
>
> Hi AI,
>
> I found the broken commit still has a question:
>
> > commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> > Author: Al Viro <[email protected]>
> > Date: Tue Jun 16 15:28:29 2020 -0400
>
> > csky: switch to ->regset_get()
>
> > NB: WTF "- what the fuck :(" is fpregs_get() playing at???
> The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
> fp regs are stored in threads' context.
> So, WTF question for?

The part under
#if defined(CONFIG_CPU_HAS_FPUV2) && !defined(CONFIG_CPU_HAS_VDSP)

What's going on there? The mapping is really weird - assuming
you had v0..v31 in the first 32 elements of regs->vr[], you
end up with

v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31

in the beginning of the output. Assuming it is the intended
behaviour, it's probably worth some comments...

2020-09-23 02:59:14

by Zhenzhong Duan

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 12:29 AM Al Viro <[email protected]> wrote:
>
> On Tue, Sep 22, 2020 at 05:15:05PM +0800, Zhenzhong Duan wrote:
> > "*" is missed in size determination as we are passing register set
> > rather than a pointer.
>
> Ack. I can push it to Linus today, unless you want it to go through
> csky tree. Preferences?

I prefer pushing to linus.

Regards
Zhenzhong

2020-09-23 03:03:25

by Guo Ren

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 8:23 AM Al Viro <[email protected]> wrote:
>
> On Wed, Sep 23, 2020 at 08:03:20AM +0800, Guo Ren wrote:
> > Thx Duan,
> >
> > Acked-by: Guo Ren <[email protected]>
> >
> > Hi AI,
> >
> > I found the broken commit still has a question:
> >
> > > commit dcad7854fcce6a2d49b6a3ead5bbefeff047e559
> > > Author: Al Viro <[email protected]>
> > > Date: Tue Jun 16 15:28:29 2020 -0400
> >
> > > csky: switch to ->regset_get()
> >
> > > NB: WTF "- what the fuck :(" is fpregs_get() playing at???
> > The fpregs_get() is for REGSET_FPR regset used by ptrace (gdb) and all
> > fp regs are stored in threads' context.
> > So, WTF question for?
>
> The part under
> #if defined(CONFIG_CPU_HAS_FPUV2) && !defined(CONFIG_CPU_HAS_VDSP)
>
> What's going on there? The mapping is really weird - assuming
> you had v0..v31 in the first 32 elements of regs->vr[], you
> end up with
>
> v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
>
> in the beginning of the output. Assuming it is the intended
> behaviour, it's probably worth some comments...
FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
regs' width is 128b.

vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
...
vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
vr[64], vr[65] = fp[16]
vr[66], vr[67] = fp[17]
...
vr[94], vr[95] = fp[31]

Yeah, this is confusing and I'll add a comment later.




--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-09-23 04:54:27

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:

> > What's going on there? The mapping is really weird - assuming
> > you had v0..v31 in the first 32 elements of regs->vr[], you
> > end up with
> >
> > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> >
> > in the beginning of the output. Assuming it is the intended
> > behaviour, it's probably worth some comments...
> FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> regs' width is 128b.
>
> vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> ...
> vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> vr[64], vr[65] = fp[16]
> vr[66], vr[67] = fp[17]
> ...
> vr[94], vr[95] = fp[31]
>
> Yeah, this is confusing and I'll add a comment later.

Umm... It would help if you described these 3 layouts:
1) kernel-side with VDSP
2) userland (identical to (1)?)
3) kernel-side without VDSP
Still confused...

PS: my apologies re commit message - I left a note to myself when doing
that series and then forgot about it ;-/

Anyway, which tree should it go through? In any case, that fix is
Acked-by: Al Viro <[email protected]>
and I can take it through vfs.git or you guys can pick in csky tree;
up to you.

2020-09-25 06:35:30

by Guo Ren

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 12:52 PM Al Viro <[email protected]> wrote:
>
> On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
>
> > > What's going on there? The mapping is really weird - assuming
> > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > end up with
> > >
> > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > >
> > > in the beginning of the output. Assuming it is the intended
> > > behaviour, it's probably worth some comments...
> > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > regs' width is 128b.
> >
> > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > ...
> > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > vr[64], vr[65] = fp[16]
> > vr[66], vr[67] = fp[17]
> > ...
> > vr[94], vr[95] = fp[31]
> >
> > Yeah, this is confusing and I'll add a comment later.
>
> Umm... It would help if you described these 3 layouts:
> 1) kernel-side with VDSP
With VDSP: we use vdsp ld/st instructions to access first 16
128bit-regs and use fpu ld/st instructions to access last 16
64bit-regs.

> 2) userland (identical to (1)?)
Identical to 1.

> 3) kernel-side without VDSP
Without VDSP: we use fpu ld/st instructions to access last 32 64bit-regs.

So, there are 96 32bit-vr[] for the struct. And with VDSP or not will
got different storage format.
With VDSP:
vr128[16] // contain first fp64[16]
fp64[16]; // second fp64[16]

Without VDSP:
fp64[32]
no-use for the reset vr[]

> Still confused...
>
> PS: my apologies re commit message - I left a note to myself when doing
> that series and then forgot about it ;-/
>
> Anyway, which tree should it go through? In any case, that fix is
Thx for your job, and pushing to linus with Zhenzhong Duan's advice.

> Acked-by: Al Viro <[email protected]>
> and I can take it through vfs.git or you guys can pick in csky tree;
> up to you.

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-12-23 02:33:25

by Zhenzhong Duan

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Sep 23, 2020 at 12:52 PM Al Viro <[email protected]> wrote:
>
> On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
>
> > > What's going on there? The mapping is really weird - assuming
> > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > end up with
> > >
> > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > >
> > > in the beginning of the output. Assuming it is the intended
> > > behaviour, it's probably worth some comments...
> > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > regs' width is 128b.
> >
> > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > ...
> > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > vr[64], vr[65] = fp[16]
> > vr[66], vr[67] = fp[17]
> > ...
> > vr[94], vr[95] = fp[31]
> >
> > Yeah, this is confusing and I'll add a comment later.
>
> Umm... It would help if you described these 3 layouts:
> 1) kernel-side with VDSP
> 2) userland (identical to (1)?)
> 3) kernel-side without VDSP
> Still confused...
>
> PS: my apologies re commit message - I left a note to myself when doing
> that series and then forgot about it ;-/
>
> Anyway, which tree should it go through? In any case, that fix is
> Acked-by: Al Viro <[email protected]>
> and I can take it through vfs.git or you guys can pick in csky tree;
> up to you.

Hi Al, Guo

Seems this patch is still pending, could you help check it?Thanks

Zhenzhong

2020-12-23 15:00:04

by Guo Ren

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

Hi Zhengzhong,

I'll take it, thx.

On Wed, Dec 23, 2020 at 10:31 AM Zhenzhong Duan
<[email protected]> wrote:
>
> On Wed, Sep 23, 2020 at 12:52 PM Al Viro <[email protected]> wrote:
> >
> > On Wed, Sep 23, 2020 at 10:37:31AM +0800, Guo Ren wrote:
> >
> > > > What's going on there? The mapping is really weird - assuming
> > > > you had v0..v31 in the first 32 elements of regs->vr[], you
> > > > end up with
> > > >
> > > > v0 v1 v2 v3 v2 v3 v6 v7 v4 v5 v10 v11 v6 v7 v14 v15
> > > > v8 v9 v18 v19 v10 v11 v22 v23 v12 v13 v26 v27 v14 v15 v30 v31
> > > >
> > > > in the beginning of the output. Assuming it is the intended
> > > > behaviour, it's probably worth some comments...
> > > FPU & VDSP use the same regs. 32 FPU regs' width is 64b and 16 VDSP
> > > regs' width is 128b.
> > >
> > > vr[0], vr[1] = fp[0] & vr[0] vr[1], vr[2], vr[3] = vdsp reg[0]
> > > ...
> > > vr[60], vr[61] = fp[15] & vr[60] vr[61], vr[62], vr[63] = vdsp reg[15]
> > > vr[64], vr[65] = fp[16]
> > > vr[66], vr[67] = fp[17]
> > > ...
> > > vr[94], vr[95] = fp[31]
> > >
> > > Yeah, this is confusing and I'll add a comment later.
> >
> > Umm... It would help if you described these 3 layouts:
> > 1) kernel-side with VDSP
> > 2) userland (identical to (1)?)
> > 3) kernel-side without VDSP
> > Still confused...
> >
> > PS: my apologies re commit message - I left a note to myself when doing
> > that series and then forgot about it ;-/
> >
> > Anyway, which tree should it go through? In any case, that fix is
> > Acked-by: Al Viro <[email protected]>
> > and I can take it through vfs.git or you guys can pick in csky tree;
> > up to you.
>
> Hi Al, Guo
>
> Seems this patch is still pending, could you help check it?Thanks
>
> Zhenzhong



--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-12-24 02:24:35

by Zhenzhong Duan

[permalink] [raw]
Subject: Re: [PATCH] csky: Fix a size determination in gpr_get()

On Wed, Dec 23, 2020 at 10:57 PM Guo Ren <[email protected]> wrote:
>
> Hi Zhengzhong,
>
> I'll take it, thx.

Thanks Guo.

Zhenzhong