Skip to contentSkip to conversation

x86: vm86 cleanups

PATCH Superseded Brian Gerst 5 participants 20 messages Jul 20, 2015 – Aug 23, 2015

Subsystems (3)
x86 x86 MM x86 entry code

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Brian Gerst

author
#0 lore raw

PATCH v3 1/1 x86: vm86 cleanups

The goal of this set of patches is to change vm86 support to return to userspace with the normal exit paths instead of leaving data on the kernel stack and jumping directly into the exit asm routines. This fixes issues like ptrace and syscall auditing not working with vm86, and makes possible cleanups in the syscall exit work code.

Changes from v2:
- Use gs slot of regs32 (present but unused in lazy mode)
- Add access_ok() checks before get_user_try/put_user_try

Changes from v1:
- Added first two patches
- Changed userspace access to copy each field explicitly instead of relying on the same order of members in the structure.

author
#1 lore raw

PATCH 1/7 x86/vm86: Clean up saved_fs/gs

There is no need to save FS and non-lazy GS outside the 32-bit regs. Lazy GS still needs to be saved because it wasn't saved on syscall entry. Save it in the gs slot of regs32, which is present but unused.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/include/asm/processor.h | 2 --
 arch/x86/kernel/vm86_32.c        | 6 ++----
 2 files changed, 2 insertions(+), 6 deletions(-)
arch/x86/include/asm/processor.h +0 -2
index 43e6519..f4e4e3f 100644
@@ -410,8 +410,6 @@ struct thread_struct {
 	unsigned long		v86flags;
 	unsigned long		v86mask;
 	unsigned long		saved_sp0;
-	unsigned int		saved_fs;
-	unsigned int		saved_gs;
 #endif
 	/* IO permissions: */
 	unsigned long		*io_bitmap_ptr;
arch/x86/kernel/vm86_32.c +2 -4
index fc9db6e..761a2f9 100644
@@ -159,8 +159,7 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 
 	ret = KVM86->regs32;
 
-	ret->fs = current->thread.saved_fs;
-	set_user_gs(ret, current->thread.saved_gs);
+	lazy_load_gs(ret->gs);
 
 	return ret;
 }

@@ -315,8 +314,7 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
  */
 	info->regs32->ax = VM86_SIGNAL;
 	tsk->thread.saved_sp0 = tsk->thread.sp0;
-	tsk->thread.saved_fs = info->regs32->fs;
-	tsk->thread.saved_gs = get_user_gs(info->regs32);
+	lazy_save_gs(info->regs32->gs);
 
 	tss = &per_cpu(cpu_tss, get_cpu());
 	tsk->thread.sp0 = (unsigned long) &info->VM86_TSS_ESP0;
Signature · 1 line
2.4.3
merge bot
#2 lore raw

[tip:x86/asm] x86/entry/vm86: Clean up saved_fs/gs

x86/entry/vm86: Clean up saved_fs/gs
Commit-ID:  0233606ce5cf12c1a0e27cb197066ea5bc2bb488
Gitweb:     http://git.kernel.org/tip/0233606ce5cf12c1a0e27cb197066ea5bc2bb488
Author:     Brian Gerst <[email protected]>
AuthorDate: Sun, 19 Jul 2015 21:09:04 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 21 Jul 2015 09:12:23 +0200

x86/entry/vm86: Clean up saved_fs/gs

There is no need to save FS and non-lazy GS outside the 32-bit regs. Lazy GS still needs to be saved because it wasn't saved on syscall entry. Save it in the gs slot of regs32, which is present but unused.

Signed-off-by Brian Gerst <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/processor.h | 2 --
 arch/x86/kernel/vm86_32.c        | 6 ++----
 2 files changed, 2 insertions(+), 6 deletions(-)
arch/x86/include/asm/processor.h +0 -2
index 43e6519..f4e4e3f 100644
@@ -410,8 +410,6 @@ struct thread_struct {
 	unsigned long		v86flags;
 	unsigned long		v86mask;
 	unsigned long		saved_sp0;
-	unsigned int		saved_fs;
-	unsigned int		saved_gs;
 #endif
 	/* IO permissions: */
 	unsigned long		*io_bitmap_ptr;
arch/x86/kernel/vm86_32.c +2 -4
index fc9db6e..761a2f9 100644
@@ -159,8 +159,7 @@ struct pt_regs *save_v86_state(struct kernel_vm86_regs *regs)
 
 	ret = KVM86->regs32;
 
-	ret->fs = current->thread.saved_fs;
-	set_user_gs(ret, current->thread.saved_gs);
+	lazy_load_gs(ret->gs);
 
 	return ret;
 }

@@ -315,8 +314,7 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
  */
 	info->regs32->ax = VM86_SIGNAL;
 	tsk->thread.saved_sp0 = tsk->thread.sp0;
-	tsk->thread.saved_fs = info->regs32->fs;
-	tsk->thread.saved_gs = get_user_gs(info->regs32);
+	lazy_save_gs(info->regs32->gs);
 
 	tss = &per_cpu(cpu_tss, get_cpu());
 	tsk->thread.sp0 = (unsigned long) &info->VM86_TSS_ESP0;
author
#3 lore raw

PATCH 2/7 x86/vm86: Preserve orig_ax

In reply to Brian Gerst

There is no legitimate reason for usermode to modify the orig_ax field on entry to vm86 mode, so copy it from the 32-bit regs.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/kernel/vm86_32.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/kernel/vm86_32.c +2 -0
index 761a2f9..9a2dc80 100644
@@ -294,6 +294,8 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
 	info->regs.pt.flags |= info->regs32->flags & ~SAFE_MASK;
 	info->regs.pt.flags |= X86_VM_MASK;
 
+	info->regs.pt.orig_ax = info->regs32->orig_ax;
+
 	switch (info->cpu_type) {
 	case CPU_286:
 		tsk->thread.v86mask = 0;
Signature · 1 line
2.4.3
merge bot
#4 lore raw

[tip:x86/asm] x86/entry/vm86: Preserve 'orig_ax'

x86/entry/vm86: Preserve 'orig_ax'
Commit-ID:  df1ae9a5dc66d9fd57109240042372b1065d984a
Gitweb:     http://git.kernel.org/tip/df1ae9a5dc66d9fd57109240042372b1065d984a
Author:     Brian Gerst <[email protected]>
AuthorDate: Sun, 19 Jul 2015 21:09:05 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 21 Jul 2015 09:12:23 +0200

x86/entry/vm86: Preserve 'orig_ax'

There is no legitimate reason for usermode to modify the 'orig_ax' field on entry to vm86 mode, so copy it from the 32-bit regs.

Signed-off-by Brian Gerst <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/kernel/vm86_32.c | 2 ++
 1 file changed, 2 insertions(+)
arch/x86/kernel/vm86_32.c +2 -0
index 761a2f9..9a2dc80 100644
@@ -294,6 +294,8 @@ static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk
 	info->regs.pt.flags |= info->regs32->flags & ~SAFE_MASK;
 	info->regs.pt.flags |= X86_VM_MASK;
 
+	info->regs.pt.orig_ax = info->regs32->orig_ax;
+
 	switch (info->cpu_type) {
 	case CPU_286:
 		tsk->thread.v86mask = 0;
author
#5 lore raw

PATCH 3/7 x86/vm86: Move userspace accesses to do_sys_vm86()

In reply to Brian Gerst

Move the userspace accesses down into the common function in preparation for the next set of patches. Also change to copying the fields explicitly instead of assuming a fixed order in pt_regs and the kernel data structures.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/include/asm/processor.h |   2 +-
 arch/x86/kernel/vm86_32.c        | 189 +++++++++++++++++++++------------------
 2 files changed, 102 insertions(+), 89 deletions(-)
arch/x86/include/asm/processor.h +1 -1
index f4e4e3f..35ad554 100644
@@ -405,7 +405,7 @@ struct thread_struct {
 	unsigned long		error_code;
 #ifdef CONFIG_X86_32
 	/* Virtual 86 mode info */
-	struct vm86_struct __user *vm86_info;
+	struct vm86plus_struct __user *vm86_info;
 	unsigned long		screen_bitmap;
 	unsigned long		v86flags;
 	unsigned long		v86mask;
arch/x86/kernel/vm86_32.c +101 -88

250 lines not on this page · raw message

Signature · 1 line
2.4.3
merge bot
#6 lore raw

[tip:x86/asm] x86/entry/vm86: Move userspace accesses to do_sys_vm86()

x86/entry/vm86: Move userspace accesses to do_sys_vm86()
Commit-ID:  ed0b2edb61ba4e557de759093d965654186f28b2
Gitweb:     http://git.kernel.org/tip/ed0b2edb61ba4e557de759093d965654186f28b2
Author:     Brian Gerst <[email protected]>
AuthorDate: Sun, 19 Jul 2015 21:09:06 -0400
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 21 Jul 2015 09:12:24 +0200

x86/entry/vm86: Move userspace accesses to do_sys_vm86()

Move the userspace accesses down into the common function in preparation for the next set of patches. Also change to copying the fields explicitly instead of assuming a fixed order in pt_regs and the kernel data structures.

Signed-off-by Brian Gerst <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/processor.h |   2 +-
 arch/x86/kernel/vm86_32.c        | 189 +++++++++++++++++++++------------------
 2 files changed, 102 insertions(+), 89 deletions(-)
arch/x86/include/asm/processor.h +1 -1
index f4e4e3f..35ad554 100644
@@ -405,7 +405,7 @@ struct thread_struct {
 	unsigned long		error_code;
 #ifdef CONFIG_X86_32
 	/* Virtual 86 mode info */
-	struct vm86_struct __user *vm86_info;
+	struct vm86plus_struct __user *vm86_info;
 	unsigned long		screen_bitmap;
 	unsigned long		v86flags;
 	unsigned long		v86mask;
arch/x86/kernel/vm86_32.c +101 -88

250 lines not on this page · raw message

author
#7 lore raw

PATCH 4/7 x86/vm86: Move vm86 fields out of thread_struct

In reply to Brian Gerst

Allocate a separate structure for the vm86 fields.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/include/asm/processor.h |  9 ++------
 arch/x86/include/asm/vm86.h      |  8 +++++++
 arch/x86/kernel/process.c        |  7 ++++++
 arch/x86/kernel/vm86_32.c        | 47 ++++++++++++++++++++++++----------------
 arch/x86/mm/fault.c              |  4 ++--
 5 files changed, 47 insertions(+), 28 deletions(-)
arch/x86/include/asm/processor.h +2 -7
index 35ad554..d636fb8 100644
@@ -403,13 +403,9 @@ struct thread_struct {
 	unsigned long		cr2;
 	unsigned long		trap_nr;
 	unsigned long		error_code;
-#ifdef CONFIG_X86_32
+#ifdef CONFIG_VM86
 	/* Virtual 86 mode info */
-	struct vm86plus_struct __user *vm86_info;
-	unsigned long		screen_bitmap;
-	unsigned long		v86flags;
-	unsigned long		v86mask;
-	unsigned long		saved_sp0;
+	struct kernel_vm86_info *vm86;
 #endif
 	/* IO permissions: */
 	unsigned long		*io_bitmap_ptr;

@@ -714,7 +710,6 @@ static inline void spin_lock_prefetch(const void *x)
 
 #define INIT_THREAD  {							  \
 	.sp0			= TOP_OF_INIT_STACK,			  \
-	.vm86_info		= NULL,					  \
 	.sysenter_cs		= __KERNEL_CS,				  \
 	.io_bitmap_ptr		= NULL,					  \
 }
arch/x86/include/asm/vm86.h +8 -0
index 1d8de3f..a02967d 100644
@@ -58,6 +58,14 @@ struct kernel_vm86_struct {
  */
 };
 
+struct kernel_vm86_info {
+	struct vm86plus_struct __user *vm86_info;
+	unsigned long screen_bitmap;
+	unsigned long v86flags;
+	unsigned long v86mask;
+	unsigned long saved_sp0;
+};
+
 #ifdef CONFIG_VM86
 
 void handle_vm86_fault(struct kernel_vm86_regs *, long);
arch/x86/kernel/process.c +7 -0
index 9cad694..5dcd037 100644
@@ -110,6 +110,13 @@ void exit_thread(void)
 		kfree(bp);
 	}
 
+#ifdef CONFIG_VM86
+	if (t->vm86) {
+		kfree(t->vm86);
+		t->vm86 = NULL;
+	}
+#endif
+
 	fpu__drop(fpu);
 }
arch/x86/kernel/vm86_32.c +28 -19

144 lines not on this page · raw message

arch/x86/mm/fault.c +2 -2
index 81dcebf..5196ac4 100644
@@ -315,12 +315,12 @@ check_v8086_mode(struct pt_regs *regs, unsigned long address,
 {
 	unsigned long bit;
 
-	if (!v8086_mode(regs))
+	if (!v8086_mode(regs) || !tsk->thread.vm86)
 		return;
 
 	bit = (address - 0xA0000) >> PAGE_SHIFT;
 	if (bit < 32)
-		tsk->thread.screen_bitmap |= 1 << bit;
+		tsk->thread.vm86->screen_bitmap |= 1 << bit;
 }
 
 static bool low_pfn(unsigned long pfn)
Signature · 1 line
2.4.3
#8 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 1 line · Brian Gerst

Allocate a separate structure for the vm86 fields.

Why is this allocated dynamically? This structure is not very large, and a hole in thread_struct isn't that big of an issue - compared to additional fragility introduced by the (mostly untested by normal apps) dynamic allocation here ...

I don't mind the introduction of the sub-structure in itself, but please embedd it in thread_struct.

Thanks,

Ingo

#9 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 12 lines · Ingo Molnar
Quoted 1 line · Brian Gerst

Allocate a separate structure for the vm86 fields.

Why is this allocated dynamically? This structure is not very large, and a hole in thread_struct isn't that big of an issue - compared to additional fragility introduced by the (mostly untested by normal apps) dynamic allocation here ...

I don't mind the introduction of the sub-structure in itself, but please embedd it in thread_struct.

This ends up being several hundred bytes, I think, due to including an entire struct pt_regs. Do we really want to enlarge thread_struct that much for something that's essentially never used?

--Andy

#10 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 11 lines · Andy Lutomirski
Quoted 10 lines · Ingo Molnar
Quoted 1 line · Brian Gerst

Allocate a separate structure for the vm86 fields.

Why is this allocated dynamically? This structure is not very large, and a hole in thread_struct isn't that big of an issue - compared to additional fragility introduced by the (mostly untested by normal apps) dynamic allocation here ...

Quoted 6 lines
Quoted 2 lines

I don't mind the introduction of the sub-structure in itself, but please embedd it in thread_struct.

This ends up being several hundred bytes, I think, due to including an entire struct pt_regs. Do we really want to enlarge thread_struct that much for something that's essentially never used?

Ok, I only judged by the first patch, I did not realize it becomes that much larger in later patches.

I've extended the changelog to explain this properly.

Thanks,

Ingo

#11 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

In reply to Ingo Molnar
Quoted 10 lines · Ingo Molnar
Quoted 1 line · Brian Gerst

Allocate a separate structure for the vm86 fields.

Why is this allocated dynamically? This structure is not very large, and a hole in thread_struct isn't that big of an issue - compared to additional fragility introduced by the (mostly untested by normal apps) dynamic allocation here ...

I don't mind the introduction of the sub-structure in itself, but please embedd it in thread_struct.

Otherwise I have no objections to the rest of the series, nice fixes!

Thanks,

Ingo

#12 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

In reply to Brian Gerst

arch/x86/kernel/process.c void exit_thread(void) · line 118

Quoted 12 lines · Brian Gerst
arch/x86/kernel/process.c +6 -0
@@ -110,6 +110,13 @@ void exit_thread(void)
 		kfree(bp);
 	}
 
+#ifdef CONFIG_VM86
+	if (t->vm86) {
+		kfree(t->vm86);
+		t->vm86 = NULL;
+	}
+#endif

This should be a helper:

	vm86__free(t->vm86)

or so.

Quoted 6 lines
arch/x86/kernel/vm86_32.c +0 -0
index e6c2b47..dce0a1c 100644

arch/x86/kernel/vm86_32.c static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus, · line 251

Quoted 9 lines
arch/x86/kernel/vm86_32.c +3 -1
@@ -242,9 +244,16 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 {
 	struct tss_struct *tss;
 	struct task_struct *tsk = current;
+	struct kernel_vm86_info *vm86 = tsk->thread.vm86;
 	unsigned long err = 0;
 
-	if (tsk->thread.saved_sp0)
+	if (!vm86)
+	{

Non-standard style.

arch/x86/kernel/vm86_32.c static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus, · line 257

Quoted 6 lines
arch/x86/kernel/vm86_32.c +5 -0
+		if (!(vm86 = kzalloc(sizeof(*vm86), GFP_KERNEL)))
+			return -ENOMEM;
+		tsk->thread.vm86 = vm86;
+	}
+	if (vm86->saved_sp0)
 		return -EPERM;

Btw., the variable names here are crazy. I had to look twice to realize that we have 'v86' and 'vm86' which are two different things.

Also, vm86plus_struct variables and fields are named wildly inconsistently:
sometimes it's 'vm86.vm86_info', sometimes it's 'v86', sometimes 'user'. Ugh.

Other fields have naming inconsistencies as well: for example we have thread.vm86->vm86plus.vm86dbg_active. 'vm86' is repeated _three_ times in that name, for no good reason.

So please clean up the naming to make this all easier to read. Only the highest level field should have 'vm86' in it - all subsequent fields will inherit that name one way or another.

At a quick glance I'd do the following renames:

struct kernel_vm86_info *vm86; => struct vm86 *vm86;

- it's obviously 'information' so the _info is superfluous.

- and it's obviously embedded in a kernel structure, so the kernel_ is superfluous as well.

Then let's look at other fields of the main structure:

struct kernel_vm86_info {
        struct vm86plus_struct __user *vm86_info;
        struct pt_regs regs32;
        unsigned long v86flags;
        unsigned long v86mask;
        unsigned long saved_sp0;

        unsigned long flags;
        unsigned long screen_bitmap;
        unsigned long cpu_type;
        struct revectored_struct int_revectored;
        struct revectored_struct int21_revectored;
        struct vm86plus_info_struct vm86plus;
};

- Why is there a vm86.flags and a vm86.v86flags field? What's the difference and can we eliminate the confusion factor?

- The fields flags..vm86plus seems to be an as-is copy of 'struct vm86plus_struct'. Could this be organized in a smarter fashion?.

- 'struct vm86_regs' appears to be an as-is copy of 32-bit pt_regs, plus:

        unsigned short es, __esh;
        unsigned short ds, __dsh;
        unsigned short fs, __fsh;
        unsigned short gs, __gsh;

    Instead of a slightly different structure copying pt_regs, why not express it 
    as:

    struct vm86_regs {
        struct pt_regs regs;

        unsigned short es, __esh;
        unsigned short ds, __dsh;
        unsigned short fs, __fsh;
        unsigned short gs, __gsh;
    };

    ?

- There's a number of 'long' fields which are always 32-bit, which is pretty confusing even if it's only ever built on 32-bit kerenls, can we use u8/u16/u32/u64 for ABI components instead?

Thanks,

Ingo

author
#13 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 54 lines · Ingo Molnar

arch/x86/kernel/process.c void exit_thread(void) · line 118

Quoted 12 lines · Brian Gerst
arch/x86/kernel/process.c +6 -0
@@ -110,6 +110,13 @@ void exit_thread(void)
              kfree(bp);
      }
 
+#ifdef CONFIG_VM86
+     if (t->vm86) {
+             kfree(t->vm86);
+             t->vm86 = NULL;
+     }
+#endif

This should be a helper:

        vm86__free(t->vm86)

or so.

Quoted 6 lines
arch/x86/kernel/vm86_32.c +0 -0
index e6c2b47..dce0a1c 100644

arch/x86/kernel/vm86_32.c static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus, · line 251

Quoted 9 lines
arch/x86/kernel/vm86_32.c +3 -1
@@ -242,9 +244,16 @@ static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus,
 {
      struct tss_struct *tss;
      struct task_struct *tsk = current;
+     struct kernel_vm86_info *vm86 = tsk->thread.vm86;
      unsigned long err = 0;
 
-     if (tsk->thread.saved_sp0)
+     if (!vm86)
+     {

Non-standard style.

arch/x86/kernel/vm86_32.c static long do_sys_vm86(struct vm86plus_struct __user *v86, bool plus, · line 257

Quoted 6 lines
arch/x86/kernel/vm86_32.c +5 -0
+             if (!(vm86 = kzalloc(sizeof(*vm86), GFP_KERNEL)))
+                     return -ENOMEM;
+             tsk->thread.vm86 = vm86;
+     }
+     if (vm86->saved_sp0)
              return -EPERM;

Btw., the variable names here are crazy. I had to look twice to realize that we have 'v86' and 'vm86' which are two different things.

Also, vm86plus_struct variables and fields are named wildly inconsistently:
sometimes it's 'vm86.vm86_info', sometimes it's 'v86', sometimes 'user'. Ugh.

Quoted 7 lines

Other fields have naming inconsistencies as well: for example we have thread.vm86->vm86plus.vm86dbg_active. 'vm86' is repeated _three_ times in that name, for no good reason.

So please clean up the naming to make this all easier to read. Only the highest level field should have 'vm86' in it - all subsequent fields will inherit that name one way or another.

Some of these field names are visible to userspace and can't change.

Quoted 8 lines

At a quick glance I'd do the following renames:

struct kernel_vm86_info *vm86; => struct vm86 *vm86;

- it's obviously 'information' so the _info is superfluous.

- and it's obviously embedded in a kernel structure, so the kernel_ is superfluous as well.

I think there is some merit to keeping the kernel prefix to make it clear it is not the userspace version, which is similar,

Quoted 17 lines

Then let's look at other fields of the main structure:

struct kernel_vm86_info {
        struct vm86plus_struct __user *vm86_info;
        struct pt_regs regs32;
        unsigned long v86flags;
        unsigned long v86mask;
        unsigned long saved_sp0;

        unsigned long flags;
        unsigned long screen_bitmap;
        unsigned long cpu_type;
        struct revectored_struct int_revectored;
        struct revectored_struct int21_revectored;
        struct vm86plus_info_struct vm86plus;
};
Quoted 2 lines

- Why is there a vm86.flags and a vm86.v86flags field? What's the difference and can we eliminate the confusion factor?

The first is a feature flag (only used for screen bitmap), and the latter is a shadow copy of EFLAGS.

Quoted 2 lines

- The fields flags..vm86plus seems to be an as-is copy of 'struct vm86plus_struct'. Could this be organized in a smarter fashion?.

I specifically want to exclude retaining a copy of vm86_regs in the kernel data. It's not needed after the syscall returns to vm86 mode. I had originally copied all the fields after regs as a whole, but Andy objected to that. So I changed it to copy all the fields individually instead, so that the order of kernel vs. userspace doesn't matter.

Quoted 18 lines

- 'struct vm86_regs' appears to be an as-is copy of 32-bit pt_regs, plus:

        unsigned short es, __esh;
        unsigned short ds, __dsh;
        unsigned short fs, __fsh;
        unsigned short gs, __gsh;

    Instead of a slightly different structure copying pt_regs, why not express it
    as:

    struct vm86_regs {
        struct pt_regs regs;

        unsigned short es, __esh;
        unsigned short ds, __dsh;
        unsigned short fs, __fsh;
        unsigned short gs, __gsh;
    };

This would be a userspace visible change. The register order and field names from userspace are fixed, and I don't want it to depend on the order of fields of the in-kernel pt_regs. It was a bad ABI design choice, but it's too late to fix it now.

Quoted 3 lines

- There's a number of 'long' fields which are always 32-bit, which is pretty confusing even if it's only ever built on 32-bit kerenls, can we use u8/u16/u32/u64 for ABI components instead?

Will do.

Quoted 4 lines

Thanks,

Ingo

--
Brian Gerst

#14 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 10 lines · Brian Gerst
Quoted 10 lines

Btw., the variable names here are crazy. I had to look twice to realize that we have 'v86' and 'vm86' which are two different things.

Also, vm86plus_struct variables and fields are named wildly inconsistently:
sometimes it's 'vm86.vm86_info', sometimes it's 'v86', sometimes 'user'. Ugh.

Other fields have naming inconsistencies as well: for example we have thread.vm86->vm86plus.vm86dbg_active. 'vm86' is repeated _three_ times in that name, for no good reason.

Quoted 5 lines
Quoted 3 lines

So please clean up the naming to make this all easier to read. Only the highest level field should have 'vm86' in it - all subsequent fields will inherit that name one way or another.

Some of these field names are visible to userspace and can't change.

That's a misconception: bits in the uapi headers can be renamed just fine.

The kernel ABI is that _semantics_ that user-space code relies on must not change.

Cleaning up field names is absolutely legit to do, and we've done it numerous times in the past. Especially where they are so confusing and inconsistent as in the vm86 code.

Thanks,

Ingo

#15 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 4 lines · Ingo Molnar
Quoted 2 lines

Some of these field names are visible to userspace and can't change.

That's a misconception: bits in the uapi headers can be renamed just fine.

I disagree. If it causes pain for user space, we just shouldn't do it. Some people copy the headers (preferred), others include the kernel header directory with a include path or a symlink, and it's damn painful if we start changing things that user space depends on.

I'd say that it's like the ABI - if people don't really notice, you can do it, but if it breaks the build of a user app, we should be very very careful. The breakage is often hard to fix because of nasty versioning issues..

Our goal in life really is "don't screw up user space".

Linus

#16 lore raw

Re: [PATCH 4/7] x86/vm86: Move vm86 fields out of thread_struct

Quoted 7 lines · Linus Torvalds
Quoted 4 lines · Ingo Molnar
Quoted 2 lines

Some of these field names are visible to userspace and can't change.

That's a misconception: bits in the uapi headers can be renamed just fine.

I disagree. If it causes pain for user space, we just shouldn't do it.

Ok, agreed - I wanted to argue against the "can't change" statement and went overboard with my own statement ...

Quite often headers can change and we've changed a number of fields in the past - but if they cause pain (as in this case) we don't do it.

So I'd say that based on past experience:

- changing small details in uapi headers is usually fine.
- changing small details in the ABI is usually not fine.

If anything breaks then the policy is the same, regardless of likelihood:
reverting the change.

Quoted 9 lines

Some people copy the headers (preferred), others include the kernel header directory with a include path or a symlink, and it's damn painful if we start changing things that user space depends on.

I'd say that it's like the ABI - if people don't really notice, you can do it, but if it breaks the build of a user app, we should be very very careful. The breakage is often hard to fix because of nasty versioning issues..

Our goal in life really is "don't screw up user space".

Yeah, agreed 100%.

Thanks,

Ingo

author
#17 lore raw

PATCH 5/7 x86/vm86: Move fields from kernel_vm86_struct

In reply to Brian Gerst

Move the non-regs fields to the off-stack data.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/include/asm/vm86.h | 16 ++++++++--------
 arch/x86/kernel/vm86_32.c   | 42 ++++++++++++++++++++++--------------------
 2 files changed, 30 insertions(+), 28 deletions(-)
arch/x86/include/asm/vm86.h +8 -8
index a02967d..ff64c27 100644
@@ -38,13 +38,7 @@ struct kernel_vm86_struct {
  * Therefore, pt_regs in fact points to a complete 'kernel_vm86_struct'
  * in kernelspace, hence we need not reget the data from userspace.
  */
-#define VM86_TSS_ESP0 flags
-	unsigned long flags;
-	unsigned long screen_bitmap;
-	unsigned long cpu_type;
-	struct revectored_struct int_revectored;
-	struct revectored_struct int21_revectored;
-	struct vm86plus_info_struct vm86plus;
+#define VM86_TSS_ESP0 regs32
 	struct pt_regs *regs32;   /* here we save the pointer to the old regs */
 /*
  * The below is not part of the structure, but the stack layout continues

@@ -60,10 +54,16 @@ struct kernel_vm86_struct {
 
 struct kernel_vm86_info {
 	struct vm86plus_struct __user *vm86_info;
-	unsigned long screen_bitmap;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;
+
+	unsigned long flags;
+	unsigned long screen_bitmap;
+	unsigned long cpu_type;
+	struct revectored_struct int_revectored;
+	struct revectored_struct int21_revectored;
+	struct vm86plus_info_struct vm86plus;
 };
 
 #ifdef CONFIG_VM86
arch/x86/kernel/vm86_32.c +22 -20

115 lines not on this page · raw message

Signature · 1 line
2.4.3
author
#18 lore raw

PATCH 6/7 x86/vm86: Eliminate kernel_vm86_struct

In reply to Brian Gerst

Now there is no vm86-specific data left on the kernel stack while in userspace, except for the 32-bit regs.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/include/asm/vm86.h | 25 +-----------
 arch/x86/kernel/vm86_32.c   | 93 +++++++++++++++++++--------------------------
 2 files changed, 41 insertions(+), 77 deletions(-)
arch/x86/include/asm/vm86.h +1 -24
index ff64c27..1740655 100644
@@ -28,32 +28,9 @@ struct kernel_vm86_regs {
 	unsigned short gs, __gsh;
 };
 
-struct kernel_vm86_struct {
-	struct kernel_vm86_regs regs;
-/*
- * the below part remains on the kernel stack while we are in VM86 mode.
- * 'tss.esp0' then contains the address of VM86_TSS_ESP0 below, and when we
- * get forced back from VM86, the CPU and "SAVE_ALL" will restore the above
- * 'struct kernel_vm86_regs' with the then actual values.
- * Therefore, pt_regs in fact points to a complete 'kernel_vm86_struct'
- * in kernelspace, hence we need not reget the data from userspace.
- */
-#define VM86_TSS_ESP0 regs32
-	struct pt_regs *regs32;   /* here we save the pointer to the old regs */
-/*
- * The below is not part of the structure, but the stack layout continues
- * this way. In front of 'return-eip' may be some data, depending on
- * compilation, so we don't rely on this and save the pointer to 'oldregs'
- * in 'regs32' above.
- * However, with GCC-2.7.2 and the current CFLAGS you see exactly this:
-
-	long return-eip;        from call to vm86()
-	struct pt_regs oldregs;  user space registers as saved by syscall
- */
-};
-
 struct kernel_vm86_info {
 	struct vm86plus_struct __user *vm86_info;
+	struct pt_regs *regs32;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;
arch/x86/kernel/vm86_32.c +40 -53

179 lines not on this page · raw message

Signature · 1 line
2.4.3
author
#19 lore raw

PATCH 7/7 x86/vm86: Use the normal pt_regs area for vm86

In reply to Brian Gerst

Change to use the normal pt_regs area to enter and exit vm86 mode. This is done by increasing the padding at the top of the stack to make room for the extra vm86 segment slots in the IRET frame. It then saves the 32-bit regs in the off-stack vm86 data, and copies in the vm86 regs. Exiting back to 32-bit mode does the reverse. This allows removing the hacks to jump directly into the exit asm code due to having to change the stack pointer. Returning normally from the vm86 syscall and the exception handlers allows things like ptrace and auditing to work properly.

Signed-off-by Brian Gerst <[email protected]>
---
 arch/x86/entry/entry_32.S          |  24 +--------
 arch/x86/include/asm/thread_info.h |  11 ++--
 arch/x86/include/asm/vm86.h        |   6 ++-
 arch/x86/kernel/signal.c           |   3 ++
 arch/x86/kernel/vm86_32.c          | 106 +++++++++++++++----------------------
 5 files changed, 58 insertions(+), 92 deletions(-)
arch/x86/entry/entry_32.S +1 -23
index 21dc60a..f940e24 100644
@@ -525,34 +525,12 @@ work_resched:
 
 work_notifysig:					# deal with pending signals and
 						# notify-resume requests
-#ifdef CONFIG_VM86
-	testl	$X86_EFLAGS_VM, PT_EFLAGS(%esp)
-	movl	%esp, %eax
-	jnz	work_notifysig_v86		# returning to kernel-space or
-						# vm86-space
-1:
-#else
-	movl	%esp, %eax
-#endif
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_NONE)
-	movb	PT_CS(%esp), %bl
-	andb	$SEGMENT_RPL_MASK, %bl
-	cmpb	$USER_RPL, %bl
-	jb	resume_kernel
+	movl	%esp, %eax
 	xorl	%edx, %edx
 	call	do_notify_resume
 	jmp	resume_userspace
-
-#ifdef CONFIG_VM86
-	ALIGN
-work_notifysig_v86:
-	pushl	%ecx				# save ti_flags for do_notify_resume
-	call	save_v86_state			# %eax contains pt_regs pointer
-	popl	%ecx
-	movl	%eax, %esp
-	jmp	1b
-#endif
 END(work_pending)
 
 	# perform syscall exit tracing
arch/x86/include/asm/thread_info.h +7 -4
index 225ee54..fdad5c2 100644
@@ -27,14 +27,17 @@
  * Without this offset, that can result in a page fault.  (We are
  * careful that, in this case, the value we read doesn't matter.)
  *
- * In vm86 mode, the hardware frame is much longer still, but we neither
- * access the extra members from NMI context, nor do we write such a
- * frame at sp0 at all.
+ * In vm86 mode, the hardware frame is much longer still, so add 16
+ * bytes to make room for the real-mode segments.
  *
  * x86_64 has a fixed-length stack frame.
  */
 #ifdef CONFIG_X86_32
-# define TOP_OF_KERNEL_STACK_PADDING 8
+# ifdef CONFIG_VM86
+#  define TOP_OF_KERNEL_STACK_PADDING 16
+# else
+#  define TOP_OF_KERNEL_STACK_PADDING 8
+# endif
 #else
 # define TOP_OF_KERNEL_STACK_PADDING 0
 #endif
arch/x86/include/asm/vm86.h +4 -2
index 1740655..8cc4a1a 100644
@@ -30,7 +30,7 @@ struct kernel_vm86_regs {
 
 struct kernel_vm86_info {
 	struct vm86plus_struct __user *vm86_info;
-	struct pt_regs *regs32;
+	struct pt_regs regs32;
 	unsigned long v86flags;
 	unsigned long v86mask;
 	unsigned long saved_sp0;

@@ -47,7 +47,7 @@ struct kernel_vm86_info {
 
 void handle_vm86_fault(struct kernel_vm86_regs *, long);
 int handle_vm86_trap(struct kernel_vm86_regs *, long, int);
-struct pt_regs *save_v86_state(struct kernel_vm86_regs *);
+void save_v86_state(struct kernel_vm86_regs *, int);
 
 struct task_struct;
 void release_vm86_irqs(struct task_struct *);

@@ -62,6 +62,8 @@ static inline int handle_vm86_trap(struct kernel_vm86_regs *a, long b, int c)
 	return 0;
 }
 
+static inline void save_v86_state(struct kernel_vm86_regs *, int) { }
+
 #endif /* CONFIG_VM86 */
 
 #endif /* _ASM_X86_VM86_H */
arch/x86/kernel/signal.c +3 -0
index 7e88cc7..bfd736e 100644
@@ -635,6 +635,9 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	bool stepping, failed;
 	struct fpu *fpu = &current->thread.fpu;
 
+	if (v8086_mode(regs))
+		save_v86_state((struct kernel_vm86_regs *) regs, VM86_SIGNAL);
+
 	/* Are we from a system call? */
 	if (syscall_get_nr(current, regs) >= 0) {
 		/* If so, check system call restarting.. */
arch/x86/kernel/vm86_32.c +43 -63

235 lines not on this page · raw message

Signature · 1 line
2.4.3