| Message ID | 20230301053425.3880773-8-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add printf and formatted asserts in the guest | expand |
On 3/1/23 13:34, Aaron Lewis wrote: > Add more flexibility to guest debugging and testing by adding > GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework. > > A buffer to hold the formatted string was added to the ucall struct. > That allows the guest/host to avoid the problem of passing an > arbitrary number of parameters between themselves when resolving the > string. Instead, the string is resolved in the guest then passed > back to the host to be logged. > > The formatted buffer is set to 1024 bytes which increases the size > of the ucall struct. As a result, this will increase the number of > pages requested for the guest. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > .../selftests/kvm/include/ucall_common.h | 19 +++++++++++++++++++ > .../testing/selftests/kvm/lib/ucall_common.c | 19 +++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h > index 0b1fde23729b..2a4400b6761a 100644 > --- a/tools/testing/selftests/kvm/include/ucall_common.h > +++ b/tools/testing/selftests/kvm/include/ucall_common.h > @@ -13,15 +13,18 @@ enum { > UCALL_NONE, > UCALL_SYNC, > UCALL_ABORT, > + UCALL_PRINTF, > UCALL_DONE, > UCALL_UNHANDLED, > }; > > #define UCALL_MAX_ARGS 7 > +#define UCALL_BUFFER_LEN 1024 > > struct ucall { > uint64_t cmd; > uint64_t args[UCALL_MAX_ARGS]; > + char buffer[UCALL_BUFFER_LEN]; Hi Aaron, A simple question, what if someone print too long in guest which exceed the UCALL_BUFFER_LEN, it seems buffer overflow will happen since vsprintf will not check the buffer length. Just in case, someone may don't know the limit and print too long. Thanks, Shaoqin > > /* Host virtual address of this struct. */ > struct ucall *hva; > @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc); > void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); > > void ucall(uint64_t cmd, int nargs, ...); > +void ucall_fmt(uint64_t cmd, const char *fmt, ...); > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); > void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa); > int ucall_header_size(void); > @@ -47,6 +51,7 @@ int ucall_header_size(void); > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) > #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage) > +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args) > #define GUEST_DONE() ucall(UCALL_DONE, 0) > > enum guest_assert_builtin_args { > @@ -56,6 +61,18 @@ enum guest_assert_builtin_args { > GUEST_ASSERT_BUILTIN_NARGS > }; > > +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...) \ > +do { \ > + if (!(_condition)) \ > + ucall_fmt(UCALL_ABORT, \ > + "Failed guest assert: " _condstr \ > + " at %s:%ld\n " format, \ > + __FILE__, __LINE__, ##_args); \ > +} while (0) > + > +#define GUEST_ASSERT_FMT(_condition, format, _args...) \ > + __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args) > + > #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) \ > do { \ > if (!(_condition)) \ > @@ -81,6 +98,8 @@ do { \ > > #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b) > > +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer) > + > #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \ > TEST_FAIL("%s at %s:%ld\n" fmt, \ > (const char *)(_ucall).args[GUEST_ERROR_STRING], \ > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > index b6a75858fe0d..92ebc5db1c41 100644 > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void) > for (i = 0; i < KVM_MAX_VCPUS; ++i) { > if (!test_and_set_bit(i, ucall_pool->in_use)) { > uc = &ucall_pool->ucalls[i]; > + uc->cmd = UCALL_NONE; > memset(uc->args, 0, sizeof(uc->args)); > + memset(uc->buffer, 0, sizeof(uc->buffer)); > return uc; > } > } > @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc) > clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); > } > > +void ucall_fmt(uint64_t cmd, const char *fmt, ...) > +{ > + struct ucall *uc; > + va_list va; > + > + uc = ucall_alloc(); > + uc->cmd = cmd; > + > + va_start(va, fmt); > + vsprintf(uc->buffer, fmt, va); > + va_end(va); > + > + ucall_arch_do_ucall((vm_vaddr_t)uc->hva); > + > + ucall_free(uc); > +} > + > void ucall(uint64_t cmd, int nargs, ...) > { > struct ucall *uc;
On Wed, Mar 1, 2023 at 12:07 AM Shaoqin Huang <shahuang@redhat.com> wrote: > > > > On 3/1/23 13:34, Aaron Lewis wrote: > > Add more flexibility to guest debugging and testing by adding > > GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework. > > > > A buffer to hold the formatted string was added to the ucall struct. > > That allows the guest/host to avoid the problem of passing an > > arbitrary number of parameters between themselves when resolving the > > string. Instead, the string is resolved in the guest then passed > > back to the host to be logged. > > > > The formatted buffer is set to 1024 bytes which increases the size > > of the ucall struct. As a result, this will increase the number of > > pages requested for the guest. > > > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > > --- > > .../selftests/kvm/include/ucall_common.h | 19 +++++++++++++++++++ > > .../testing/selftests/kvm/lib/ucall_common.c | 19 +++++++++++++++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h > > index 0b1fde23729b..2a4400b6761a 100644 > > --- a/tools/testing/selftests/kvm/include/ucall_common.h > > +++ b/tools/testing/selftests/kvm/include/ucall_common.h > > @@ -13,15 +13,18 @@ enum { > > UCALL_NONE, > > UCALL_SYNC, > > UCALL_ABORT, > > + UCALL_PRINTF, > > UCALL_DONE, > > UCALL_UNHANDLED, > > }; > > > > #define UCALL_MAX_ARGS 7 > > +#define UCALL_BUFFER_LEN 1024 > > > > struct ucall { > > uint64_t cmd; > > uint64_t args[UCALL_MAX_ARGS]; > > + char buffer[UCALL_BUFFER_LEN]; > Hi Aaron, > > A simple question, what if someone print too long in guest which exceed > the UCALL_BUFFER_LEN, it seems buffer overflow will happen since > vsprintf will not check the buffer length. > > Just in case, someone may don't know the limit and print too long. > > Thanks, > Shaoqin > > In the followup I can check the length of the string written in ucall_fmt() and return an overflow assert to the host instead if one is detected. > > /* Host virtual address of this struct. */ > > struct ucall *hva; > > @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc); > > void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); > > > > void ucall(uint64_t cmd, int nargs, ...); > > +void ucall_fmt(uint64_t cmd, const char *fmt, ...); > > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); > > void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa); > > int ucall_header_size(void); > > @@ -47,6 +51,7 @@ int ucall_header_size(void); > > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ > > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) > > #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage) > > +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args) > > #define GUEST_DONE() ucall(UCALL_DONE, 0) > > > > enum guest_assert_builtin_args { > > @@ -56,6 +61,18 @@ enum guest_assert_builtin_args { > > GUEST_ASSERT_BUILTIN_NARGS > > }; > > > > +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...) \ > > +do { \ > > + if (!(_condition)) \ > > + ucall_fmt(UCALL_ABORT, \ > > + "Failed guest assert: " _condstr \ > > + " at %s:%ld\n " format, \ > > + __FILE__, __LINE__, ##_args); \ > > +} while (0) > > + > > +#define GUEST_ASSERT_FMT(_condition, format, _args...) \ > > + __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args) > > + > > #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) \ > > do { \ > > if (!(_condition)) \ > > @@ -81,6 +98,8 @@ do { \ > > > > #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b) > > > > +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer) > > + > > #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \ > > TEST_FAIL("%s at %s:%ld\n" fmt, \ > > (const char *)(_ucall).args[GUEST_ERROR_STRING], \ > > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > > index b6a75858fe0d..92ebc5db1c41 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > > @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void) > > for (i = 0; i < KVM_MAX_VCPUS; ++i) { > > if (!test_and_set_bit(i, ucall_pool->in_use)) { > > uc = &ucall_pool->ucalls[i]; > > + uc->cmd = UCALL_NONE; > > memset(uc->args, 0, sizeof(uc->args)); > > + memset(uc->buffer, 0, sizeof(uc->buffer)); > > return uc; > > } > > } > > @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc) > > clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); > > } > > > > +void ucall_fmt(uint64_t cmd, const char *fmt, ...) > > +{ > > + struct ucall *uc; > > + va_list va; > > + > > + uc = ucall_alloc(); > > + uc->cmd = cmd; > > + > > + va_start(va, fmt); > > + vsprintf(uc->buffer, fmt, va); > > + va_end(va); > > + > > + ucall_arch_do_ucall((vm_vaddr_t)uc->hva); > > + > > + ucall_free(uc); > > +} > > + > > void ucall(uint64_t cmd, int nargs, ...) > > { > > struct ucall *uc; >
On Wed, Mar 01, 2023, Aaron Lewis wrote: > Add more flexibility to guest debugging and testing by adding > GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework. > > A buffer to hold the formatted string was added to the ucall struct. > That allows the guest/host to avoid the problem of passing an > arbitrary number of parameters between themselves when resolving the > string. Instead, the string is resolved in the guest then passed > back to the host to be logged. > > The formatted buffer is set to 1024 bytes which increases the size > of the ucall struct. As a result, this will increase the number of > pages requested for the guest. Why 1024? I don't particuarly have an opinion, but some explanation of where this magic number comes from would be helpful. > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > .../selftests/kvm/include/ucall_common.h | 19 +++++++++++++++++++ > .../testing/selftests/kvm/lib/ucall_common.c | 19 +++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h > index 0b1fde23729b..2a4400b6761a 100644 > --- a/tools/testing/selftests/kvm/include/ucall_common.h > +++ b/tools/testing/selftests/kvm/include/ucall_common.h > @@ -13,15 +13,18 @@ enum { > UCALL_NONE, > UCALL_SYNC, > UCALL_ABORT, > + UCALL_PRINTF, > UCALL_DONE, > UCALL_UNHANDLED, > }; > > #define UCALL_MAX_ARGS 7 > +#define UCALL_BUFFER_LEN 1024 > > struct ucall { > uint64_t cmd; > uint64_t args[UCALL_MAX_ARGS]; > + char buffer[UCALL_BUFFER_LEN]; > > /* Host virtual address of this struct. */ > struct ucall *hva; > @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc); > void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); > > void ucall(uint64_t cmd, int nargs, ...); > +void ucall_fmt(uint64_t cmd, const char *fmt, ...); > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); > void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa); > int ucall_header_size(void); > @@ -47,6 +51,7 @@ int ucall_header_size(void); > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) > #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage) > +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args) > #define GUEST_DONE() ucall(UCALL_DONE, 0) > > enum guest_assert_builtin_args { > @@ -56,6 +61,18 @@ enum guest_assert_builtin_args { > GUEST_ASSERT_BUILTIN_NARGS > }; > > +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...) \ > +do { \ > + if (!(_condition)) \ > + ucall_fmt(UCALL_ABORT, \ > + "Failed guest assert: " _condstr \ > + " at %s:%ld\n " format, \ Please don't wrap strings, especially not in macros. Just run past 80 chars if necessary. > + __FILE__, __LINE__, ##_args); \ > +} while (0) > + > +#define GUEST_ASSERT_FMT(_condition, format, _args...) \ > + __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args) > + > #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) \ > do { \ > if (!(_condition)) \ > @@ -81,6 +98,8 @@ do { \ > > #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b) > > +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer) > + > #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \ > TEST_FAIL("%s at %s:%ld\n" fmt, \ > (const char *)(_ucall).args[GUEST_ERROR_STRING], \ > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c > index b6a75858fe0d..92ebc5db1c41 100644 > --- a/tools/testing/selftests/kvm/lib/ucall_common.c > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c > @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void) > for (i = 0; i < KVM_MAX_VCPUS; ++i) { > if (!test_and_set_bit(i, ucall_pool->in_use)) { > uc = &ucall_pool->ucalls[i]; > + uc->cmd = UCALL_NONE; This is unnecessary, there's one caller and it immediately sets cmd. If it's a sticking point, force the caller to pass in @cmd and move the WRITE_ONCE() here. Oh, and if you do that, put it in a separate patch. > memset(uc->args, 0, sizeof(uc->args)); > + memset(uc->buffer, 0, sizeof(uc->buffer)); > return uc; > }
diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h index 0b1fde23729b..2a4400b6761a 100644 --- a/tools/testing/selftests/kvm/include/ucall_common.h +++ b/tools/testing/selftests/kvm/include/ucall_common.h @@ -13,15 +13,18 @@ enum { UCALL_NONE, UCALL_SYNC, UCALL_ABORT, + UCALL_PRINTF, UCALL_DONE, UCALL_UNHANDLED, }; #define UCALL_MAX_ARGS 7 +#define UCALL_BUFFER_LEN 1024 struct ucall { uint64_t cmd; uint64_t args[UCALL_MAX_ARGS]; + char buffer[UCALL_BUFFER_LEN]; /* Host virtual address of this struct. */ struct ucall *hva; @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc); void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); void ucall(uint64_t cmd, int nargs, ...); +void ucall_fmt(uint64_t cmd, const char *fmt, ...); uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa); int ucall_header_size(void); @@ -47,6 +51,7 @@ int ucall_header_size(void); #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \ ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4) #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage) +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args) #define GUEST_DONE() ucall(UCALL_DONE, 0) enum guest_assert_builtin_args { @@ -56,6 +61,18 @@ enum guest_assert_builtin_args { GUEST_ASSERT_BUILTIN_NARGS }; +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...) \ +do { \ + if (!(_condition)) \ + ucall_fmt(UCALL_ABORT, \ + "Failed guest assert: " _condstr \ + " at %s:%ld\n " format, \ + __FILE__, __LINE__, ##_args); \ +} while (0) + +#define GUEST_ASSERT_FMT(_condition, format, _args...) \ + __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args) + #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) \ do { \ if (!(_condition)) \ @@ -81,6 +98,8 @@ do { \ #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b) +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer) + #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \ TEST_FAIL("%s at %s:%ld\n" fmt, \ (const char *)(_ucall).args[GUEST_ERROR_STRING], \ diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c index b6a75858fe0d..92ebc5db1c41 100644 --- a/tools/testing/selftests/kvm/lib/ucall_common.c +++ b/tools/testing/selftests/kvm/lib/ucall_common.c @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void) for (i = 0; i < KVM_MAX_VCPUS; ++i) { if (!test_and_set_bit(i, ucall_pool->in_use)) { uc = &ucall_pool->ucalls[i]; + uc->cmd = UCALL_NONE; memset(uc->args, 0, sizeof(uc->args)); + memset(uc->buffer, 0, sizeof(uc->buffer)); return uc; } } @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc) clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use); } +void ucall_fmt(uint64_t cmd, const char *fmt, ...) +{ + struct ucall *uc; + va_list va; + + uc = ucall_alloc(); + uc->cmd = cmd; + + va_start(va, fmt); + vsprintf(uc->buffer, fmt, va); + va_end(va); + + ucall_arch_do_ucall((vm_vaddr_t)uc->hva); + + ucall_free(uc); +} + void ucall(uint64_t cmd, int nargs, ...) { struct ucall *uc;
Add more flexibility to guest debugging and testing by adding GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework. A buffer to hold the formatted string was added to the ucall struct. That allows the guest/host to avoid the problem of passing an arbitrary number of parameters between themselves when resolving the string. Instead, the string is resolved in the guest then passed back to the host to be logged. The formatted buffer is set to 1024 bytes which increases the size of the ucall struct. As a result, this will increase the number of pages requested for the guest. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- .../selftests/kvm/include/ucall_common.h | 19 +++++++++++++++++++ .../testing/selftests/kvm/lib/ucall_common.c | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+)