[PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Dmitry Safonov]

As we have here core registers, use them to determine application's
mode and sizes of register set and elf_prstatus instead of TIF_IA32
flag.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 arch/x86/include/asm/compat.h |  8 ++++----
 fs/binfmt_elf.c               | 18 ++++++++++--------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 5a3b2c119ed0..d0b517fc77ff 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -264,10 +264,10 @@ struct compat_shmid64_ds {
 #ifdef CONFIG_X86_X32_ABI
 typedef struct user_regs_struct compat_elf_gregset_t;
 
-#define PR_REG_SIZE(S) (test_thread_flag(TIF_IA32) ? 68 : 216)
-#define PRSTATUS_SIZE(S) (test_thread_flag(TIF_IA32) ? 144 : 296)
-#define SET_PR_FPVALID(S,V) \
-  do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0)) = (V); } \
+#define PR_REG_SIZE(S, R) (!user_64bit_mode(R) ? 68 : 216)
+#define PRSTATUS_SIZE(S, R) (!user_64bit_mode(R) ? 144 : 296)
+#define SET_PR_FPVALID(S, V, R) \
+  do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0, R)) = (V); } \
   while (0)
 
 #define COMPAT_USE_64BIT_TIME \
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index e158b22ef32f..3876382edc72 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1623,11 +1623,11 @@ static void do_thread_regset_writeback(struct task_struct *task,
 }
 
 #ifndef PR_REG_SIZE
-#define PR_REG_SIZE(S) sizeof(S)
+#define PR_REG_SIZE(S, R) sizeof(S)
 #endif
 
 #ifndef PRSTATUS_SIZE
-#define PRSTATUS_SIZE(S) sizeof(S)
+#define PRSTATUS_SIZE(S, R) sizeof(S)
 #endif
 
 #ifndef PR_REG_PTR
@@ -1635,12 +1635,13 @@ static void do_thread_regset_writeback(struct task_struct *task,
 #endif
 
 #ifndef SET_PR_FPVALID
-#define SET_PR_FPVALID(S, V) ((S)->pr_fpvalid = (V))
+#define SET_PR_FPVALID(S, V, R) ((S)->pr_fpvalid = (V))
 #endif
 
 static int fill_thread_core_info(struct elf_thread_core_info *t,
 				 const struct user_regset_view *view,
-				 long signr, size_t *total)
+				 long signr, size_t *total,
+				 struct pt_regs *regs __maybe_unused)
 {
 	unsigned int i;
 
@@ -1652,11 +1653,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 	 */
 	fill_prstatus(&t->prstatus, t->task, signr);
 	(void) view->regsets[0].get(t->task, &view->regsets[0],
-				    0, PR_REG_SIZE(t->prstatus.pr_reg),
+				    0, PR_REG_SIZE(t->prstatus.pr_reg, regs),
 				    PR_REG_PTR(&t->prstatus), NULL);
 
 	fill_note(&t->notes[0], "CORE", NT_PRSTATUS,
-		  PRSTATUS_SIZE(t->prstatus), &t->prstatus);
+		  PRSTATUS_SIZE(t->prstatus, regs), &t->prstatus);
 	*total += notesize(&t->notes[0]);
 
 	do_thread_regset_writeback(t->task, &view->regsets[0]);
@@ -1686,7 +1687,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 						  regset->core_note_type,
 						  size, data);
 				else {
-					SET_PR_FPVALID(&t->prstatus, 1);
+					SET_PR_FPVALID(&t->prstatus, 1, regs);
 					fill_note(&t->notes[i], "CORE",
 						  NT_PRFPREG, size, data);
 				}
@@ -1772,7 +1773,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	 * Now fill in each thread's information.
 	 */
 	for (t = info->thread; t != NULL; t = t->next)
-		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
+		if (!fill_thread_core_info(t, view, siginfo->si_signo,
+					&info->size, regs))
 			return 0;
 
 	/*
-- 
2.8.2

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Cyrill Gorcunov]

On Wed, Jun 01, 2016 at 04:11:35PM +0300, Dmitry Safonov wrote:
> As we have here core registers, use them to determine application's
> mode and sizes of register set and elf_prstatus instead of TIF_IA32
> flag.
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>

This can be used unrelated to vdso re-mapping idea, right?

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Dmitry Safonov]

On 06/03/2016 12:51 PM, Cyrill Gorcunov wrote:
> On Wed, Jun 01, 2016 at 04:11:35PM +0300, Dmitry Safonov wrote:
>> As we have here core registers, use them to determine application's
>> mode and sizes of register set and elf_prstatus instead of TIF_IA32
>> flag.
>>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: linux-fsdevel@vger.kernel.org
>> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
>
> This can be used unrelated to vdso re-mapping idea, right?
>

Right -- 4,5,6 are pathes for dropping TIF_IA32 flag.
I put them in this patches set because of their goal,
to make tasks independent of presence of TIF_IA32.

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Cyrill Gorcunov]

On Fri, Jun 03, 2016 at 12:56:30PM +0300, Dmitry Safonov wrote:
> > 
> > This can be used unrelated to vdso re-mapping idea, right?
> > 
> 
> Right -- 4,5,6 are pathes for dropping TIF_IA32 flag.
> I put them in this patches set because of their goal,
> to make tasks independent of presence of TIF_IA32.

Ah, great. So this is continues Andy's idea of
ripping off this flag. Thanks!

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Oleg Nesterov]

On 06/01, Dmitry Safonov wrote:
>
>  static int fill_thread_core_info(struct elf_thread_core_info *t,
>  				 const struct user_regset_view *view,
> -				 long signr, size_t *total)
> +				 long signr, size_t *total,
> +				 struct pt_regs *regs __maybe_unused)
>  {
>  	unsigned int i;
>  
> @@ -1652,11 +1653,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>  	 */
>  	fill_prstatus(&t->prstatus, t->task, signr);
>  	(void) view->regsets[0].get(t->task, &view->regsets[0],
> -				    0, PR_REG_SIZE(t->prstatus.pr_reg),
> +				    0, PR_REG_SIZE(t->prstatus.pr_reg, regs),

Hmm. I don't understand this... Note that this "regs" argument has nothing
to do with t->task if the process is multithreaded,

> @@ -1772,7 +1773,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
>  	 * Now fill in each thread's information.
>  	 */
>  	for (t = info->thread; t != NULL; t = t->next)
> -		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
> +		if (!fill_thread_core_info(t, view, siginfo->si_signo,
> +					&info->size, regs))

fill_note_info(..., args) is called with args = task_pt_regs(dumper_thread).

Oleg.

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Oleg Nesterov]

On 06/06, Oleg Nesterov wrote:
>
> On 06/01, Dmitry Safonov wrote:
> >
> >  static int fill_thread_core_info(struct elf_thread_core_info *t,
> >  				 const struct user_regset_view *view,
> > -				 long signr, size_t *total)
> > +				 long signr, size_t *total,
> > +				 struct pt_regs *regs __maybe_unused)
> >  {
> >  	unsigned int i;
> >
> > @@ -1652,11 +1653,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
> >  	 */
> >  	fill_prstatus(&t->prstatus, t->task, signr);
> >  	(void) view->regsets[0].get(t->task, &view->regsets[0],
> > -				    0, PR_REG_SIZE(t->prstatus.pr_reg),
> > +				    0, PR_REG_SIZE(t->prstatus.pr_reg, regs),
>
> Hmm. I don't understand this... Note that this "regs" argument has nothing
> to do with t->task if the process is multithreaded,
>
> > @@ -1772,7 +1773,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
> >  	 * Now fill in each thread's information.
> >  	 */
> >  	for (t = info->thread; t != NULL; t = t->next)
> > -		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
> > +		if (!fill_thread_core_info(t, view, siginfo->si_signo,
> > +					&info->size, regs))
>
> fill_note_info(..., args) is called with args = task_pt_regs(dumper_thread).

forgot to mention... yes, this matches the fact we use a single "view"
for all threads, and we get it via task_user_regset_view(dump_task).

But this change (imo) adds even more confusion, and without the next patch
the logic looks "obviously wrong", becauase PR_REG_SIZE/etc look at
dumper_thread->cs while task_user_regset_view() checks thread flags.

Anyway I fail to understand these macros... Say, PR_REG_SIZE(S). Can't we
kill it and use regsets[0].n * regsets[0].size instead ? These numbers
should match whatever we do, if we call ->get().

Oleg.

Re: [PATCH 4/6] x86/coredump: use core regs, rather that TIF_IA32 flag

[Dmitry Safonov]

On 06/07/2016 01:43 AM, Oleg Nesterov wrote:
> On 06/06, Oleg Nesterov wrote:
>>
>> On 06/01, Dmitry Safonov wrote:
>>>
>>>  static int fill_thread_core_info(struct elf_thread_core_info *t,
>>>  				 const struct user_regset_view *view,
>>> -				 long signr, size_t *total)
>>> +				 long signr, size_t *total,
>>> +				 struct pt_regs *regs __maybe_unused)
>>>  {
>>>  	unsigned int i;
>>>
>>> @@ -1652,11 +1653,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>>>  	 */
>>>  	fill_prstatus(&t->prstatus, t->task, signr);
>>>  	(void) view->regsets[0].get(t->task, &view->regsets[0],
>>> -				    0, PR_REG_SIZE(t->prstatus.pr_reg),
>>> +				    0, PR_REG_SIZE(t->prstatus.pr_reg, regs),
>>
>> Hmm. I don't understand this... Note that this "regs" argument has nothing
>> to do with t->task if the process is multithreaded,
>>
>>> @@ -1772,7 +1773,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
>>>  	 * Now fill in each thread's information.
>>>  	 */
>>>  	for (t = info->thread; t != NULL; t = t->next)
>>> -		if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
>>> +		if (!fill_thread_core_info(t, view, siginfo->si_signo,
>>> +					&info->size, regs))
>>
>> fill_note_info(..., args) is called with args = task_pt_regs(dumper_thread).
>
> forgot to mention... yes, this matches the fact we use a single "view"
> for all threads, and we get it via task_user_regset_view(dump_task).
>
> But this change (imo) adds even more confusion, and without the next patch
> the logic looks "obviously wrong", becauase PR_REG_SIZE/etc look at
> dumper_thread->cs while task_user_regset_view() checks thread flags.
>
> Anyway I fail to understand these macros... Say, PR_REG_SIZE(S). Can't we
> kill it and use regsets[0].n * regsets[0].size instead ? These numbers
> should match whatever we do, if we call ->get().
>

Thanks, the idea of dropping PR_REG_SIZE looks better than my patch!
I'll try to drop those macros for the next revision.

-- 
Regards,
Dmitry Safonov