[PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[Andy Shevchenko]

guid_t is internal type of the kernel which is mistakenly had been exposed
to the user space. Replace it with raw buffers.

Besides that it's obvious from the link in the sample code, that the real
use is of uuid_t type, but because uuid_t hasn't exposed, so should guid_t,
the code includes and uses wrong type.

Fixes: 5b06931d7f8b ("sample/acrn: Introduce a sample of HSM ioctl interface usage")
Fixes: 9c5137aedd11 ("virt: acrn: Introduce VM management interfaces")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/uapi/linux/acrn.h | 3 +--
 samples/acrn/vm-sample.c  | 5 ++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
index 353b2a2e4536..4312af1b8013 100644
--- a/include/uapi/linux/acrn.h
+++ b/include/uapi/linux/acrn.h
@@ -12,7 +12,6 @@
 #define _UAPI_ACRN_H
 
 #include <linux/types.h>
-#include <linux/uuid.h>
 
 #define ACRN_IO_REQUEST_MAX		16
 
@@ -198,7 +197,7 @@ struct acrn_vm_creation {
 	__u16	reserved0;
 	__u16	vcpu_num;
 	__u16	reserved1;
-	guid_t	uuid;
+	__u8	uuid[16];
 	__u64	vm_flag;
 	__u64	ioreq_buf;
 	__u64	cpu_affinity;
diff --git a/samples/acrn/vm-sample.c b/samples/acrn/vm-sample.c
index b2dad47a77a0..1639fcadc766 100644
--- a/samples/acrn/vm-sample.c
+++ b/samples/acrn/vm-sample.c
@@ -30,7 +30,10 @@ static struct acrn_io_request *io_req_buf = (struct acrn_io_request *)io_request
 __u16 vcpu_num;
 __u16 vmid;
 /* POST_STANDARD_VM_UUID1, refer to https://github.com/projectacrn/acrn-hypervisor/blob/master/hypervisor/include/common/vm_uuids.h */
-guid_t vm_uuid = GUID_INIT(0x385479d2, 0xd625, 0xe811, 0x86, 0x4e, 0xcb, 0x7a, 0x18, 0xb3, 0x46, 0x43);
+__u8 vm_uuid[16] = {
+	0xd2, 0x79, 0x54, 0x38, 0x25, 0xd6, 0x11, 0xe8,
+	0x86, 0x4e, 0xcb, 0x7a, 0x18, 0xb3, 0x46, 0x43,
+};
 
 int hsm_fd;
 int is_running = 1;
-- 
2.33.0

Re: [PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[Greg Kroah-Hartman]

On Fri, Oct 01, 2021 at 04:56:44PM +0300, Andy Shevchenko wrote:
> guid_t is internal type of the kernel which is mistakenly had been exposed
> to the user space. Replace it with raw buffers.

Wait, why is it a mistake to expose it to userspace?  What does this
conflict with?  Is it a namespace issue?  Or something else?

thanks,

greg k-h

Re: [PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[Andy Shevchenko]

On Fri, Oct 01, 2021 at 04:01:41PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 01, 2021 at 04:56:44PM +0300, Andy Shevchenko wrote:
> > guid_t is internal type of the kernel which is mistakenly had been exposed
> > to the user space. Replace it with raw buffers.
> 
> Wait, why is it a mistake to expose it to userspace?  What does this
> conflict with?  Is it a namespace issue?  Or something else?

It is the type which is defined solely for kernel use and what user space
should do is to use types defined by respective libraries, such as libuuid.

If you read the commit message to the end, you will notice that I have
mentioned the possible scenario what happened here. They seems misusing
guid_t as uuid_t, but the latter is not available for the users from
the kernel headers (and this is good). So this is an exact example why
guid_t shouldn't be exposed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[Greg Kroah-Hartman]

On Fri, Oct 01, 2021 at 05:58:49PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 01, 2021 at 04:01:41PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Oct 01, 2021 at 04:56:44PM +0300, Andy Shevchenko wrote:
> > > guid_t is internal type of the kernel which is mistakenly had been exposed
> > > to the user space. Replace it with raw buffers.
> > 
> > Wait, why is it a mistake to expose it to userspace?  What does this
> > conflict with?  Is it a namespace issue?  Or something else?
> 
> It is the type which is defined solely for kernel use and what user space
> should do is to use types defined by respective libraries, such as libuuid.

But that does not work here.  We want to expose a common uuid type that
userspace can use in these structures, as well as the kernel.  Do you
really want to "open code" arrays all over the place?

> If you read the commit message to the end, you will notice that I have
> mentioned the possible scenario what happened here. They seems misusing
> guid_t as uuid_t, but the latter is not available for the users from
> the kernel headers (and this is good). So this is an exact example why
> guid_t shouldn't be exposed.

Then we should create a type that we can use here.  The kernel can not
use libuuid, so what is wrong with using the kernel variable type
namespace for this?  __guid?  __uuid?

thanks,

greg k-h

Re: [PATCH v1 1/1] virt: acrn: Drop internal kernel type from ABI

[Andy Shevchenko]

On Fri, Oct 01, 2021 at 05:10:10PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Oct 01, 2021 at 05:58:49PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 01, 2021 at 04:01:41PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Oct 01, 2021 at 04:56:44PM +0300, Andy Shevchenko wrote:
> > > > guid_t is internal type of the kernel which is mistakenly had been exposed
> > > > to the user space. Replace it with raw buffers.
> > > 
> > > Wait, why is it a mistake to expose it to userspace?  What does this
> > > conflict with?  Is it a namespace issue?  Or something else?
> > 
> > It is the type which is defined solely for kernel use and what user space
> > should do is to use types defined by respective libraries, such as libuuid.
> 
> But that does not work here.  We want to expose a common uuid type that
> userspace can use in these structures, as well as the kernel.  Do you
> really want to "open code" arrays all over the place?

It's not used "all over a place", the only code is recently added by this
driver.

> > If you read the commit message to the end, you will notice that I have
> > mentioned the possible scenario what happened here. They seems misusing
> > guid_t as uuid_t, but the latter is not available for the users from
> > the kernel headers (and this is good). So this is an exact example why
> > guid_t shouldn't be exposed.
> 
> Then we should create a type that we can use here.  The kernel can not
> use libuuid, so what is wrong with using the kernel variable type
> namespace for this?  __guid?  __uuid?

Why? "All over the place" except this code (okay and one more, i.e. MEI,
but this is another story) we use raw arrays in ABI. Why virt out of a
sudden need a special _kernel_ type for that? I think the author simply
missed this subtle detail that it's not supposed to be outside of the
kernel.

-- 
With Best Regards,
Andy Shevchenko