[PATCH v1] mei: Don't encourage to use kernel internal types in user code

[PATCH v1] mei: Don't encourage to use kernel internal types in user code

[Andy Shevchenko]

uuid_le is internal kernel type which shall not be exposed to the user
in the first place. In order to mitigate the (wrong) distribution of
the use of that type, switch MEI AMT sample to plain unsigned char array.

Note, there is no ABI change involved.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 samples/mei/mei-amt-version.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/samples/mei/mei-amt-version.c b/samples/mei/mei-amt-version.c
index 32234481ad7d..458cb6db57c6 100644
--- a/samples/mei/mei-amt-version.c
+++ b/samples/mei/mei-amt-version.c
@@ -90,7 +90,7 @@
 } while (0)
 
 struct mei {
-	uuid_le guid;
+	unsigned char guid[16];
 	bool initialized;
 	bool verbose;
 	unsigned int buf_size;
@@ -108,7 +108,7 @@ static void mei_deinit(struct mei *cl)
 	cl->initialized = false;
 }
 
-static bool mei_init(struct mei *me, const uuid_le *guid,
+static bool mei_init(struct mei *me, const unsigned char *guid,
 		unsigned char req_protocol_version, bool verbose)
 {
 	int result;
@@ -126,7 +126,7 @@ static bool mei_init(struct mei *me, const uuid_le *guid,
 	memset(&data, 0, sizeof(data));
 	me->initialized = true;
 
-	memcpy(&data.in_client_uuid, &me->guid, sizeof(me->guid));
+	memcpy(&data.in_client_uuid, me->guid, sizeof(me->guid));
 	result = ioctl(me->fd, IOCTL_MEI_CONNECT_CLIENT, &data);
 	if (result) {
 		mei_err(me, "IOCTL_MEI_CONNECT_CLIENT receive message. err=%d\n", result);
@@ -270,8 +270,11 @@ struct amt_host_if_resp_header {
 	unsigned char data[0];
 } __attribute__((packed));
 
-const uuid_le MEI_IAMTHIF = UUID_LE(0x12f80028, 0xb4b7, 0x4b2d,  \
-				0xac, 0xa8, 0x46, 0xe0, 0xff, 0x65, 0x81, 0x4c);
+/* MEI AMT Interface GUID: 12f80028-b4b7-4b2d-aca8-46e0ff65814c */
+const unsigned char mei_iamthif[16] = {
+	0x28, 0x00, 0xf8, 0x12, 0xb7, 0xb4, 0x2d, 0x4b,
+	0xac, 0xa8, 0x46, 0xe0, 0xff, 0x65, 0x81, 0x4c,
+};
 
 #define AMT_HOST_IF_CODE_VERSIONS_REQUEST  0x0400001A
 #define AMT_HOST_IF_CODE_VERSIONS_RESPONSE 0x0480001A
@@ -295,7 +298,7 @@ static bool amt_host_if_init(struct amt_host_if *acmd,
 		      unsigned long send_timeout, bool verbose)
 {
 	acmd->send_timeout = (send_timeout) ? send_timeout : 20000;
-	acmd->initialized = mei_init(&acmd->mei_cl, &MEI_IAMTHIF, 0, verbose);
+	acmd->initialized = mei_init(&acmd->mei_cl, mei_iamthif, 0, verbose);
 	return acmd->initialized;
 }
 
-- 
2.25.0

RE: [PATCH v1] mei: Don't encourage to use kernel internal types in user code

[Winkler, Tomas]

> 
> uuid_le is internal kernel type which shall not be exposed to the user in the first
> place. 
Why, these types are exported via include/uapi/linux/uuid.h

In order to mitigate the (wrong) distribution of the use of that type,
> switch MEI AMT sample to plain unsigned char array.

There was a change to guid_t from uuild_le, anyhow there is much more code
 except this sample that uses those types. 

Nack so far.
Tomas

> 
> Note, there is no ABI change involved.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  samples/mei/mei-amt-version.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/samples/mei/mei-amt-version.c b/samples/mei/mei-amt-version.c
> index 32234481ad7d..458cb6db57c6 100644
> --- a/samples/mei/mei-amt-version.c
> +++ b/samples/mei/mei-amt-version.c
> @@ -90,7 +90,7 @@
>  } while (0)
> 
>  struct mei {
> -	uuid_le guid;
> +	unsigned char guid[16];
>  	bool initialized;
>  	bool verbose;
>  	unsigned int buf_size;
> @@ -108,7 +108,7 @@ static void mei_deinit(struct mei *cl)
>  	cl->initialized = false;
>  }
> 
> -static bool mei_init(struct mei *me, const uuid_le *guid,
> +static bool mei_init(struct mei *me, const unsigned char *guid,
>  		unsigned char req_protocol_version, bool verbose)  {
>  	int result;
> @@ -126,7 +126,7 @@ static bool mei_init(struct mei *me, const uuid_le
> *guid,
>  	memset(&data, 0, sizeof(data));
>  	me->initialized = true;
> 
> -	memcpy(&data.in_client_uuid, &me->guid, sizeof(me->guid));
> +	memcpy(&data.in_client_uuid, me->guid, sizeof(me->guid));
>  	result = ioctl(me->fd, IOCTL_MEI_CONNECT_CLIENT, &data);
>  	if (result) {
>  		mei_err(me, "IOCTL_MEI_CONNECT_CLIENT receive message.
> err=%d\n", result); @@ -270,8 +270,11 @@ struct amt_host_if_resp_header {
>  	unsigned char data[0];
>  } __attribute__((packed));
> 
> -const uuid_le MEI_IAMTHIF = UUID_LE(0x12f80028, 0xb4b7, 0x4b2d,  \
> -				0xac, 0xa8, 0x46, 0xe0, 0xff, 0x65, 0x81,
> 0x4c);
> +/* MEI AMT Interface GUID: 12f80028-b4b7-4b2d-aca8-46e0ff65814c */
> +const unsigned char mei_iamthif[16] = {
> +	0x28, 0x00, 0xf8, 0x12, 0xb7, 0xb4, 0x2d, 0x4b,
> +	0xac, 0xa8, 0x46, 0xe0, 0xff, 0x65, 0x81, 0x4c, };
> 
>  #define AMT_HOST_IF_CODE_VERSIONS_REQUEST  0x0400001A  #define
> AMT_HOST_IF_CODE_VERSIONS_RESPONSE 0x0480001A @@ -295,7 +298,7
> @@ static bool amt_host_if_init(struct amt_host_if *acmd,
>  		      unsigned long send_timeout, bool verbose)  {
>  	acmd->send_timeout = (send_timeout) ? send_timeout : 20000;
> -	acmd->initialized = mei_init(&acmd->mei_cl, &MEI_IAMTHIF, 0,
> verbose);
> +	acmd->initialized = mei_init(&acmd->mei_cl, mei_iamthif, 0, verbose);
>  	return acmd->initialized;
>  }
> 
> --
> 2.25.0

Re: [PATCH v1] mei: Don't encourage to use kernel internal types in user code

[Andy Shevchenko]

+Cc: Christoph.

On Sat, Feb 29, 2020 at 04:28:11PM +0000, Winkler, Tomas wrote:
> > uuid_le is internal kernel type which shall not be exposed to the user in the first
> > place. 
> Why, these types are exported via include/uapi/linux/uuid.h

Which is wrong from the day 1.

The uuid_t type is being provided by libuuid in the user space, there is no
(more) kernel exported equivalent. Same should be done to the uuid_le.

We already discussed this couple of years ago.

> In order to mitigate the (wrong) distribution of the use of that type,
> > switch MEI AMT sample to plain unsigned char array.
> 
> There was a change to guid_t from uuild_le, anyhow there is much more code
>  except this sample that uses those types. 

I guess you misunderstood the point. The types are for kernel use and keeping them
exported in a condition like it's now (quoter baked due to drop of uuid_be part
completely and uuid_le partially) is wrong.

There is *no* ABI change. And basically libuuid or another one should provide
type and infrastructure for this.

> Nack so far.

If you would like to bear the legacy type, why not to move this UUID UAPI parts
directly to MEI?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v1] mei: Don't encourage to use kernel internal types in user code

[Winkler, Tomas]

> 
> +Cc: Christoph.
> 
> On Sat, Feb 29, 2020 at 04:28:11PM +0000, Winkler, Tomas wrote:
> > > uuid_le is internal kernel type which shall not be exposed to the
> > > user in the first place.
> > Why, these types are exported via include/uapi/linux/uuid.h
> 
> Which is wrong from the day 1.
I'm not sure why, this is API between kernel and the user space. 

> The uuid_t type is being provided by libuuid in the user space, there is no
> (more) kernel exported equivalent. Same should be done to the uuid_le.

There are many uuid libraries, which  is the one that provides the uuid type
between kernel and the user space? 

> 
> We already discussed this couple of years ago.
I do not recall be part of this conversation, please share the link. 

> 
> > In order to mitigate the (wrong) distribution of the use of that type,
> > > switch MEI AMT sample to plain unsigned char array.
> >
> > There was a change to guid_t from uuild_le, anyhow there is much more
> > code  except this sample that uses those types.
> 
> I guess you misunderstood the point. The types are for kernel use and keeping
> them exported in a condition like it's now (quoter baked due to drop of uuid_be
> part completely and uuid_le partially) is wrong.
Is wrong how... ?  What is broken in the concept ? Please give me an example of what is going to wrong, here. 
Just saying that something is wrong is not convincing. 

> There is *no* ABI change. And basically libuuid or another one should provide
> type and infrastructure for this.
But API is already out there, do you plan to remove it? 
  
> > Nack so far.
> 
> If you would like to bear the legacy type, why not to move this UUID UAPI parts
> directly to MEI?
I can but do you know all the software that includes <linux/uuid.h> ?
Thanks
Tomas