[PATCH] x86/traps: Fix failed ASSERT() in do_guest_trap()

[PATCH] x86/traps: Fix failed ASSERT() in do_guest_trap()

[Andrew Cooper]

c/s 2e426d6 "x86/traps: Drop use_error_code parameter from do_{,guest_}trap()"
introduced an assertion which covered the correctness of shifting 1u by an
input parameter.

While all other inputs provide a constants vector, the `int $N` handling path
from do_general_protection() passes any vector.

This path is triggered by XTF, which uses `int 0x20` to facilitate returning
to kernel mode after running specific tests in user mode.

No vectors above 32 have an error code, so adjust the logic to cope.

Reported-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>

RFC:

1) Should we start making use of Linux's "Fixes:" tag?

2) I considered using TRAP_nr instead of 32, but "(trapnr < TRAP_nr)" is odd,
and it hides the visual correctness of seeing that 1u is never shifted out of
range.
---
 xen/arch/x86/traps.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c228b45..e822719 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -631,11 +631,8 @@ static void do_guest_trap(unsigned int trapnr,
     struct vcpu *v = current;
     struct trap_bounce *tb;
     const struct trap_info *ti;
-    bool_t use_error_code;
-
-    ASSERT(trapnr < 32);
-
-    use_error_code = (TRAP_HAVE_EC & (1u << trapnr));
+    const bool use_error_code =
+        ((trapnr < 32) && (TRAP_HAVE_EC & (1u << trapnr)));
 
     trace_pv_trap(trapnr, regs->eip, use_error_code, regs->error_code);
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86/traps: Fix failed ASSERT() in do_guest_trap()

[Jan Beulich]

>>> On 10.08.16 at 11:53, <andrew.cooper3@citrix.com> wrote:
> c/s 2e426d6 "x86/traps: Drop use_error_code parameter from do_{,guest_}trap()"
> introduced an assertion which covered the correctness of shifting 1u by an
> input parameter.
> 
> While all other inputs provide a constants vector, the `int $N` handling path
> from do_general_protection() passes any vector.
> 
> This path is triggered by XTF, which uses `int 0x20` to facilitate returning
> to kernel mode after running specific tests in user mode.
> 
> No vectors above 32 have an error code, so adjust the logic to cope.
> 
> Reported-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> RFC:
> 
> 1) Should we start making use of Linux's "Fixes:" tag?

It wouldn't hurt if someone did, but mentioning the offending commit
in the description would seem enough (and often better) to me. In no
case would I like seeing just a commit ID, without also the title being
quoted, as that will always mean one has to consult the repo for
understanding what exact change was broken (that is, of course,
only for those of us who can't memorize all the commit ID-s and their
respective titles of the last few years).

> 2) I considered using TRAP_nr instead of 32, but "(trapnr < TRAP_nr)" is odd,
> and it hides the visual correctness of seeing that 1u is never shifted out of
> range.

Either variant is fine with me.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel