linux-kernel - Re: [PATCH 04/10] KVM: selftests: Add GUEST_ASSERT variants to pass values to host

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Apr 2020 18:02:19 +0200
From:   Andrew Jones <drjones@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Peter Xu <peterx@...hat.com>,
        Wainer dos Santos Moschetta <wainersm@...hat.com>
Subject: Re: [PATCH 04/10] KVM: selftests: Add GUEST_ASSERT variants to pass
 values to host

On Fri, Apr 10, 2020 at 04:17:01PM -0700, Sean Christopherson wrote:
> Add variants of GUEST_ASSERT to pass values back to the host, e.g. to
> help debug/understand a failure when the the cause of the assert isn't
> necessarily binary.
> 
> It'd probably be possible to auto-calculate the number of arguments and
> just have a single GUEST_ASSERT, but there are a limited number of
> variants and silently eating arguments could lead to subtle code bugs.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  | 25 +++++++++++++++----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index d4c3e4d9cd92..e38d91bd8ec1 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -313,11 +313,26 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc);
>  
>  #define GUEST_SYNC(stage)	ucall(UCALL_SYNC, 2, "hello", stage)
>  #define GUEST_DONE()		ucall(UCALL_DONE, 0)
> -#define GUEST_ASSERT(_condition) do {			\
> -	if (!(_condition))				\
> -		ucall(UCALL_ABORT, 2,			\
> -			"Failed guest assert: "		\
> -			#_condition, __LINE__);		\
> +#define __GUEST_ASSERT(_condition, _nargs, _args...) do {	\
> +	if (!(_condition))					\
> +		ucall(UCALL_ABORT, 2 + _nargs,			\

Need () around _nargs

> +			"Failed guest assert: "			\
> +			#_condition, __LINE__, _args);		\
>  } while (0)

We can free up another arg and add __FILE__. Something like the following
(untested):

 #include "linux/stringify.h"

 #define __GUEST_ASSERT(_condition, _nargs, _args...) do {                            \
       if (!(_condition))                                                             \
               ucall(UCALL_ABORT, (_nargs) + 1,                                       \
                     "Failed guest assert: "                                          \
                     #_condition " at " __FILE__ ":" __stringify(__LINE__), _args);   \
 } while (0)

>  
> +#define GUEST_ASSERT(_condition) \
> +	__GUEST_ASSERT((_condition), 0, 0)
> +
> +#define GUEST_ASSERT_1(_condition, arg1) \
> +	__GUEST_ASSERT((_condition), 1, (arg1))
> +
> +#define GUEST_ASSERT_2(_condition, arg1, arg2) \
> +	__GUEST_ASSERT((_condition), 2, (arg1), (arg2))
> +
> +#define GUEST_ASSERT_3(_condition, arg1, arg2, arg3) \
> +	__GUEST_ASSERT((_condition), 3, (arg1), (arg2), (arg3))
> +
> +#define GUEST_ASSERT_4(_condition, arg1, arg2, arg3, arg4) \
> +	__GUEST_ASSERT((_condition), 4, (arg1), (arg2), (arg3), (arg4))
> +

nit: don't need the () around any of the macro params above

>  #endif /* SELFTEST_KVM_UTIL_H */
> -- 
> 2.26.0
> 

We could instead add test specific ucalls. To do so, we should first add
UCALL_TEST_SPECIFIC = 32 to the ucall enum, and then tests could extend it

 enum {
  MY_UCALL_1 = UCALL_TEST_SPECIFIC,
  MY_UCALL_2,
 };

With appropriately named test specific ucalls it may allow for clearer
code. At least GUEST_ASSERT_<N> wouldn't take on new meanings for each
test.

Thanks,
drew