All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] mei: Avoid the use of one-element arrays
@ 2020-07-14 21:45 Gustavo A. R. Silva
  2020-07-22 18:27 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-14 21:45 UTC (permalink / raw)
  To: Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman
  Cc: linux-kernel, Gustavo A. R. Silva

One-element arrays are being deprecated[1]. Replace the one-element
arrays with a simple value type u8 reserved, once this is just a
placeholder for alignment.

Also, while there, use the preferred form for passing a size of a struct.
The alternative form where struct name is spelled out hurts readability
and introduces an opportunity for a bug when the variable type is changed
but the corresponding sizeof that is passed as argument is not.

[1] https://github.com/KSPP/linux/issues/79

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Use a more concise changelog text.

 drivers/misc/mei/hbm.c | 4 ++--
 drivers/misc/mei/hw.h  | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
index a44094cdbc36..f020d5594154 100644
--- a/drivers/misc/mei/hbm.c
+++ b/drivers/misc/mei/hbm.c
@@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device *dev, u8 addr, u8 status)
 {
 	struct mei_msg_hdr mei_hdr;
 	struct hbm_add_client_response resp;
-	const size_t len = sizeof(struct hbm_add_client_response);
+	const size_t len = sizeof(resp);
 	int ret;
 
 	dev_dbg(dev->dev, "adding client response\n");
 
 	mei_hbm_hdr(&mei_hdr, len);
 
-	memset(&resp, 0, sizeof(struct hbm_add_client_response));
+	memset(&resp, 0, len);
 	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
 	resp.me_addr = addr;
 	resp.status  = status;
diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h
index b1a8d5ec88b3..8c0297f0e7f3 100644
--- a/drivers/misc/mei/hw.h
+++ b/drivers/misc/mei/hw.h
@@ -346,13 +346,13 @@ struct hbm_add_client_request {
  * @hbm_cmd: bus message command header
  * @me_addr: address of the client in ME
  * @status: if HBMS_SUCCESS then the client can now accept connections.
- * @reserved: reserved
+ * @reserved: reserved for alignment.
  */
 struct hbm_add_client_response {
 	u8 hbm_cmd;
 	u8 me_addr;
 	u8 status;
-	u8 reserved[1];
+	u8 reserved;
 } __packed;
 
 /**
@@ -461,7 +461,7 @@ struct hbm_notification {
 	u8 hbm_cmd;
 	u8 me_addr;
 	u8 host_addr;
-	u8 reserved[1];
+	u8 reserved;
 } __packed;
 
 /**
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-14 21:45 [PATCH v2] mei: Avoid the use of one-element arrays Gustavo A. R. Silva
@ 2020-07-22 18:27 ` Gustavo A. R. Silva
  2020-07-22 18:49   ` Greg Kroah-Hartman
  2020-07-22 19:04   ` Winkler, Tomas
  0 siblings, 2 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-22 18:27 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Tomas Winkler, Arnd Bergmann, Greg Kroah-Hartman
  Cc: linux-kernel

Hi all,

Friendly ping: who can take this? :)

Thanks
--
Gustavo

On 7/14/20 16:45, Gustavo A. R. Silva wrote:
> One-element arrays are being deprecated[1]. Replace the one-element
> arrays with a simple value type u8 reserved, once this is just a
> placeholder for alignment.
> 
> Also, while there, use the preferred form for passing a size of a struct.
> The alternative form where struct name is spelled out hurts readability
> and introduces an opportunity for a bug when the variable type is changed
> but the corresponding sizeof that is passed as argument is not.
> 
> [1] https://github.com/KSPP/linux/issues/79
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Changes in v2:
>  - Use a more concise changelog text.
> 
>  drivers/misc/mei/hbm.c | 4 ++--
>  drivers/misc/mei/hw.h  | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
> index a44094cdbc36..f020d5594154 100644
> --- a/drivers/misc/mei/hbm.c
> +++ b/drivers/misc/mei/hbm.c
> @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device *dev, u8 addr, u8 status)
>  {
>  	struct mei_msg_hdr mei_hdr;
>  	struct hbm_add_client_response resp;
> -	const size_t len = sizeof(struct hbm_add_client_response);
> +	const size_t len = sizeof(resp);
>  	int ret;
>  
>  	dev_dbg(dev->dev, "adding client response\n");
>  
>  	mei_hbm_hdr(&mei_hdr, len);
>  
> -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
> +	memset(&resp, 0, len);
>  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
>  	resp.me_addr = addr;
>  	resp.status  = status;
> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h
> index b1a8d5ec88b3..8c0297f0e7f3 100644
> --- a/drivers/misc/mei/hw.h
> +++ b/drivers/misc/mei/hw.h
> @@ -346,13 +346,13 @@ struct hbm_add_client_request {
>   * @hbm_cmd: bus message command header
>   * @me_addr: address of the client in ME
>   * @status: if HBMS_SUCCESS then the client can now accept connections.
> - * @reserved: reserved
> + * @reserved: reserved for alignment.
>   */
>  struct hbm_add_client_response {
>  	u8 hbm_cmd;
>  	u8 me_addr;
>  	u8 status;
> -	u8 reserved[1];
> +	u8 reserved;
>  } __packed;
>  
>  /**
> @@ -461,7 +461,7 @@ struct hbm_notification {
>  	u8 hbm_cmd;
>  	u8 me_addr;
>  	u8 host_addr;
> -	u8 reserved[1];
> +	u8 reserved;
>  } __packed;
>  
>  /**
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 18:27 ` Gustavo A. R. Silva
@ 2020-07-22 18:49   ` Greg Kroah-Hartman
  2020-07-22 19:00     ` Gustavo A. R. Silva
  2020-07-22 19:04   ` Winkler, Tomas
  1 sibling, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-22 18:49 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, Tomas Winkler, Arnd Bergmann, linux-kernel

On Wed, Jul 22, 2020 at 01:27:10PM -0500, Gustavo A. R. Silva wrote:
> Hi all,
> 
> Friendly ping: who can take this? :)

I will, give me a chance...

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 18:49   ` Greg Kroah-Hartman
@ 2020-07-22 19:00     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-22 19:00 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Gustavo A. R. Silva, Tomas Winkler, Arnd Bergmann, linux-kernel

Hey Greg,

On 7/22/20 13:49, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 01:27:10PM -0500, Gustavo A. R. Silva wrote:
>> Hi all,
>>
>> Friendly ping: who can take this? :)
> 
> I will, give me a chance...
> 

Oops, sorry... I just wanted to make sure this makes it to 5.9-rc1. :)

Thanks!
--
Gustavo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 18:27 ` Gustavo A. R. Silva
  2020-07-22 18:49   ` Greg Kroah-Hartman
@ 2020-07-22 19:04   ` Winkler, Tomas
  2020-07-22 19:29     ` Gustavo A. R. Silva
  1 sibling, 1 reply; 8+ messages in thread
From: Winkler, Tomas @ 2020-07-22 19:04 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Gustavo A. R. Silva, Arnd Bergmann,
	Greg Kroah-Hartman
  Cc: linux-kernel


> 
> Hi all,
> 
> Friendly ping: who can take this? :)
> 
> Thanks
> --
> Gustavo
> 
> On 7/14/20 16:45, Gustavo A. R. Silva wrote:
> > One-element arrays are being deprecated[1]. Replace the one-element
> > arrays with a simple value type u8 reserved, once this is just a
> > placeholder for alignment.
> >
> > Also, while there, use the preferred form for passing a size of a struct.
> > The alternative form where struct name is spelled out hurts
> > readability and introduces an opportunity for a bug when the variable
> > type is changed but the corresponding sizeof that is passed as argument is
> not.
> >
> > [1] https://github.com/KSPP/linux/issues/79
> >
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> > Changes in v2:
> >  - Use a more concise changelog text.
> >
> >  drivers/misc/mei/hbm.c | 4 ++--
> >  drivers/misc/mei/hw.h  | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> > a44094cdbc36..f020d5594154 100644
> > --- a/drivers/misc/mei/hbm.c
> > +++ b/drivers/misc/mei/hbm.c
> > @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device
> > *dev, u8 addr, u8 status)  {
> >  	struct mei_msg_hdr mei_hdr;
> >  	struct hbm_add_client_response resp;
> > -	const size_t len = sizeof(struct hbm_add_client_response);
> > +	const size_t len = sizeof(resp);
> >  	int ret;
> >
> >  	dev_dbg(dev->dev, "adding client response\n");
> >
> >  	mei_hbm_hdr(&mei_hdr, len);
> >
> > -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
> > +	memset(&resp, 0, len);
> >  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> >  	resp.me_addr = addr;
> >  	resp.status  = status;

This should be probably in a different patch it's not related to the second part.

> > diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> > b1a8d5ec88b3..8c0297f0e7f3 100644
> > --- a/drivers/misc/mei/hw.h
> > +++ b/drivers/misc/mei/hw.h
I have second thoughts of this part as all reserved fields in this file are of form u8 reserved[X], 
so we will lose that uniformity with this change, you have to look at the file as whole
not just at the patch.  So I prefer we drop that part of the patch. 

> > @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> >   * @hbm_cmd: bus message command header
> >   * @me_addr: address of the client in ME
> >   * @status: if HBMS_SUCCESS then the client can now accept connections.
> > - * @reserved: reserved
> > + * @reserved: reserved for alignment.
> >   */
> >  struct hbm_add_client_response {
> >  	u8 hbm_cmd;
> >  	u8 me_addr;
> >  	u8 status;
> > -	u8 reserved[1];
> > +	u8 reserved;
> >  } __packed;
> >
> >  /**
> > @@ -461,7 +461,7 @@ struct hbm_notification {
> >  	u8 hbm_cmd;
> >  	u8 me_addr;
> >  	u8 host_addr;
> > -	u8 reserved[1];
> > +	u8 reserved;
> >  } __packed;
> >
> >  /**
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 19:04   ` Winkler, Tomas
@ 2020-07-22 19:29     ` Gustavo A. R. Silva
  2020-07-22 22:40       ` Winkler, Tomas
  0 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-22 19:29 UTC (permalink / raw)
  To: Winkler, Tomas, Gustavo A. R. Silva, Arnd Bergmann, Greg Kroah-Hartman
  Cc: linux-kernel, Kees Cook

Hi Tomas,

Please, see my comments below...

On 7/22/20 14:04, Winkler, Tomas wrote:
> 
>>
>> Hi all,
>>
>> Friendly ping: who can take this? :)
>>
>> Thanks
>> --
>> Gustavo
>>
>> On 7/14/20 16:45, Gustavo A. R. Silva wrote:
>>> One-element arrays are being deprecated[1]. Replace the one-element
>>> arrays with a simple value type u8 reserved, once this is just a
>>> placeholder for alignment.
>>>
>>> Also, while there, use the preferred form for passing a size of a struct.
>>> The alternative form where struct name is spelled out hurts
>>> readability and introduces an opportunity for a bug when the variable
>>> type is changed but the corresponding sizeof that is passed as argument is
>> not.
>>>
>>> [1] https://github.com/KSPP/linux/issues/79
>>>
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>> ---
>>> Changes in v2:
>>>  - Use a more concise changelog text.
>>>
>>>  drivers/misc/mei/hbm.c | 4 ++--
>>>  drivers/misc/mei/hw.h  | 6 +++---
>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
>>> a44094cdbc36..f020d5594154 100644
>>> --- a/drivers/misc/mei/hbm.c
>>> +++ b/drivers/misc/mei/hbm.c
>>> @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device
>>> *dev, u8 addr, u8 status)  {
>>>  	struct mei_msg_hdr mei_hdr;
>>>  	struct hbm_add_client_response resp;
>>> -	const size_t len = sizeof(struct hbm_add_client_response);
>>> +	const size_t len = sizeof(resp);
>>>  	int ret;
>>>
>>>  	dev_dbg(dev->dev, "adding client response\n");
>>>
>>>  	mei_hbm_hdr(&mei_hdr, len);
>>>
>>> -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
>>> +	memset(&resp, 0, len);
>>>  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
>>>  	resp.me_addr = addr;
>>>  	resp.status  = status;
> 
> This should be probably in a different patch it's not related to the second part.
> 
>>> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
>>> b1a8d5ec88b3..8c0297f0e7f3 100644
>>> --- a/drivers/misc/mei/hw.h
>>> +++ b/drivers/misc/mei/hw.h
> I have second thoughts of this part as all reserved fields in this file are of form u8 reserved[X], 
> so we will lose that uniformity with this change, you have to look at the file as whole
> not just at the patch.  So I prefer we drop that part of the patch. 
> 

This is actually the main point of this patch: the removal of one-element arrays.
And yeah, every place in the kernel that uses the form that you mention will see
it's uniformity slightly modified, and that's for a good cause: the removal of
one-element arrays, so we can enable bounds checking.

Thanks
--
Gustavo

>>> @@ -346,13 +346,13 @@ struct hbm_add_client_request {
>>>   * @hbm_cmd: bus message command header
>>>   * @me_addr: address of the client in ME
>>>   * @status: if HBMS_SUCCESS then the client can now accept connections.
>>> - * @reserved: reserved
>>> + * @reserved: reserved for alignment.
>>>   */
>>>  struct hbm_add_client_response {
>>>  	u8 hbm_cmd;
>>>  	u8 me_addr;
>>>  	u8 status;
>>> -	u8 reserved[1];
>>> +	u8 reserved;
>>>  } __packed;
>>>
>>>  /**
>>> @@ -461,7 +461,7 @@ struct hbm_notification {
>>>  	u8 hbm_cmd;
>>>  	u8 me_addr;
>>>  	u8 host_addr;
>>> -	u8 reserved[1];
>>> +	u8 reserved;
>>>  } __packed;
>>>
>>>  /**
>>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 19:29     ` Gustavo A. R. Silva
@ 2020-07-22 22:40       ` Winkler, Tomas
  2020-07-22 23:01         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 8+ messages in thread
From: Winkler, Tomas @ 2020-07-22 22:40 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Gustavo A. R. Silva, Arnd Bergmann,
	Greg Kroah-Hartman
  Cc: linux-kernel, Kees Cook

> 
> Hi Tomas,
> 
> Please, see my comments below...
> 
> On 7/22/20 14:04, Winkler, Tomas wrote:
> >
> >>
> >> Hi all,
> >>
> >> Friendly ping: who can take this? :)
> >>
> >> Thanks
> >> --
> >> Gustavo
> >>
> >> On 7/14/20 16:45, Gustavo A. R. Silva wrote:
> >>> One-element arrays are being deprecated[1]. Replace the one-element
> >>> arrays with a simple value type u8 reserved, once this is just a
> >>> placeholder for alignment.
> >>>
> >>> Also, while there, use the preferred form for passing a size of a struct.
> >>> The alternative form where struct name is spelled out hurts
> >>> readability and introduces an opportunity for a bug when the
> >>> variable type is changed but the corresponding sizeof that is passed
> >>> as argument is
> >> not.
> >>>
> >>> [1] https://github.com/KSPP/linux/issues/79
> >>>
> >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >>> ---
> >>> Changes in v2:
> >>>  - Use a more concise changelog text.
> >>>
> >>>  drivers/misc/mei/hbm.c | 4 ++--
> >>>  drivers/misc/mei/hw.h  | 6 +++---
> >>>  2 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> >>> a44094cdbc36..f020d5594154 100644
> >>> --- a/drivers/misc/mei/hbm.c
> >>> +++ b/drivers/misc/mei/hbm.c
> >>> @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct
> >>> mei_device *dev, u8 addr, u8 status)  {
> >>>  	struct mei_msg_hdr mei_hdr;
> >>>  	struct hbm_add_client_response resp;
> >>> -	const size_t len = sizeof(struct hbm_add_client_response);
> >>> +	const size_t len = sizeof(resp);
> >>>  	int ret;
> >>>
> >>>  	dev_dbg(dev->dev, "adding client response\n");
> >>>
> >>>  	mei_hbm_hdr(&mei_hdr, len);
> >>>
> >>> -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
> >>> +	memset(&resp, 0, len);
> >>>  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> >>>  	resp.me_addr = addr;
> >>>  	resp.status  = status;
> >
> > This should be probably in a different patch it's not related to the second
> part.


Frankly I will post other version of this that cleans the whole file. 

> >
> >>> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> >>> b1a8d5ec88b3..8c0297f0e7f3 100644
> >>> --- a/drivers/misc/mei/hw.h
> >>> +++ b/drivers/misc/mei/hw.h
> > I have second thoughts of this part as all reserved fields in this
> > file are of form u8 reserved[X], so we will lose that uniformity with
> > this change, you have to look at the file as whole not just at the patch.  So I
> prefer we drop that part of the patch.
> >
> 
> This is actually the main point of this patch: the removal of one-element
> arrays.
> And yeah, every place in the kernel that uses the form that you mention will
> see it's uniformity slightly modified, and that's for a good cause: the removal
> of one-element arrays, so we can enable bounds checking.

I was going over https://github.com/KSPP/linux/issues/79, I'm not sure this all related  to flexible arrays,
those are just reserved struct members. So because it's hard to identify a legitimate usage of single element arrays
we are going to kill them all? It's more esthetic / readability issue here but there might be some legit use case for one element array, no?


> 
> Thanks
> --
> Gustavo
> 
> >>> @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> >>>   * @hbm_cmd: bus message command header
> >>>   * @me_addr: address of the client in ME
> >>>   * @status: if HBMS_SUCCESS then the client can now accept
> connections.
> >>> - * @reserved: reserved
> >>> + * @reserved: reserved for alignment.
> >>>   */
> >>>  struct hbm_add_client_response {
> >>>  	u8 hbm_cmd;
> >>>  	u8 me_addr;
> >>>  	u8 status;
> >>> -	u8 reserved[1];
> >>> +	u8 reserved;
> >>>  } __packed;
> >>>
> >>>  /**
> >>> @@ -461,7 +461,7 @@ struct hbm_notification {
> >>>  	u8 hbm_cmd;
> >>>  	u8 me_addr;
> >>>  	u8 host_addr;
> >>> -	u8 reserved[1];
> >>> +	u8 reserved;
> >>>  } __packed;
> >>>
> >>>  /**
> >>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] mei: Avoid the use of one-element arrays
  2020-07-22 22:40       ` Winkler, Tomas
@ 2020-07-22 23:01         ` Gustavo A. R. Silva
  0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2020-07-22 23:01 UTC (permalink / raw)
  To: Winkler, Tomas, Gustavo A. R. Silva, Arnd Bergmann, Greg Kroah-Hartman
  Cc: linux-kernel, Kees Cook



On 7/22/20 17:40, Winkler, Tomas wrote:
>>
>> Hi Tomas,
>>
>> Please, see my comments below...
>>
>> On 7/22/20 14:04, Winkler, Tomas wrote:
>>>
>>>>
>>>> Hi all,
>>>>
>>>> Friendly ping: who can take this? :)
>>>>
>>>> Thanks
>>>> --
>>>> Gustavo
>>>>
>>>> On 7/14/20 16:45, Gustavo A. R. Silva wrote:
>>>>> One-element arrays are being deprecated[1]. Replace the one-element
>>>>> arrays with a simple value type u8 reserved, once this is just a
>>>>> placeholder for alignment.
>>>>>
>>>>> Also, while there, use the preferred form for passing a size of a struct.
>>>>> The alternative form where struct name is spelled out hurts
>>>>> readability and introduces an opportunity for a bug when the
>>>>> variable type is changed but the corresponding sizeof that is passed
>>>>> as argument is
>>>> not.
>>>>>
>>>>> [1] https://github.com/KSPP/linux/issues/79
>>>>>
>>>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>>>>> ---
>>>>> Changes in v2:
>>>>>  - Use a more concise changelog text.
>>>>>
>>>>>  drivers/misc/mei/hbm.c | 4 ++--
>>>>>  drivers/misc/mei/hw.h  | 6 +++---
>>>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
>>>>> a44094cdbc36..f020d5594154 100644
>>>>> --- a/drivers/misc/mei/hbm.c
>>>>> +++ b/drivers/misc/mei/hbm.c
>>>>> @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct
>>>>> mei_device *dev, u8 addr, u8 status)  {
>>>>>  	struct mei_msg_hdr mei_hdr;
>>>>>  	struct hbm_add_client_response resp;
>>>>> -	const size_t len = sizeof(struct hbm_add_client_response);
>>>>> +	const size_t len = sizeof(resp);
>>>>>  	int ret;
>>>>>
>>>>>  	dev_dbg(dev->dev, "adding client response\n");
>>>>>
>>>>>  	mei_hbm_hdr(&mei_hdr, len);
>>>>>
>>>>> -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
>>>>> +	memset(&resp, 0, len);
>>>>>  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
>>>>>  	resp.me_addr = addr;
>>>>>  	resp.status  = status;
>>>
>>> This should be probably in a different patch it's not related to the second
>> part.
> 
> 
> Frankly I will post other version of this that cleans the whole file. 
> 

Sounds good. :)

>>>
>>>>> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
>>>>> b1a8d5ec88b3..8c0297f0e7f3 100644
>>>>> --- a/drivers/misc/mei/hw.h
>>>>> +++ b/drivers/misc/mei/hw.h
>>> I have second thoughts of this part as all reserved fields in this
>>> file are of form u8 reserved[X], so we will lose that uniformity with
>>> this change, you have to look at the file as whole not just at the patch.  So I
>> prefer we drop that part of the patch.
>>>
>>
>> This is actually the main point of this patch: the removal of one-element
>> arrays.
>> And yeah, every place in the kernel that uses the form that you mention will
>> see it's uniformity slightly modified, and that's for a good cause: the removal
>> of one-element arrays, so we can enable bounds checking.
> 
> I was going over https://github.com/KSPP/linux/issues/79, I'm not sure this all related  to flexible arrays,

I've opened a new issue for this:

https://github.com/KSPP/linux/issues/86

And I'm already including the link above in these sorts
of patches; please, see:

https://lore.kernel.org/lkml/20200717215500.GA13910@embeddedor/

> those are just reserved struct members. So because it's hard to identify a legitimate usage of single element arrays
> we are going to kill them all? It's more esthetic / readability issue here but there might be some legit use case for one element array, no?
> 

The code is continuously evolving and, if a one-element array is not intended
to be used as a variable-length array at all, I frankly cannot think of any another
use that is not merely esthetic/readability. :)

Thanks
--
Gustavo

> 
>>
>> Thanks
>> --
>> Gustavo
>>
>>>>> @@ -346,13 +346,13 @@ struct hbm_add_client_request {
>>>>>   * @hbm_cmd: bus message command header
>>>>>   * @me_addr: address of the client in ME
>>>>>   * @status: if HBMS_SUCCESS then the client can now accept
>> connections.
>>>>> - * @reserved: reserved
>>>>> + * @reserved: reserved for alignment.
>>>>>   */
>>>>>  struct hbm_add_client_response {
>>>>>  	u8 hbm_cmd;
>>>>>  	u8 me_addr;
>>>>>  	u8 status;
>>>>> -	u8 reserved[1];
>>>>> +	u8 reserved;
>>>>>  } __packed;
>>>>>
>>>>>  /**
>>>>> @@ -461,7 +461,7 @@ struct hbm_notification {
>>>>>  	u8 hbm_cmd;
>>>>>  	u8 me_addr;
>>>>>  	u8 host_addr;
>>>>> -	u8 reserved[1];
>>>>> +	u8 reserved;
>>>>>  } __packed;
>>>>>
>>>>>  /**
>>>>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-22 23:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14 21:45 [PATCH v2] mei: Avoid the use of one-element arrays Gustavo A. R. Silva
2020-07-22 18:27 ` Gustavo A. R. Silva
2020-07-22 18:49   ` Greg Kroah-Hartman
2020-07-22 19:00     ` Gustavo A. R. Silva
2020-07-22 19:04   ` Winkler, Tomas
2020-07-22 19:29     ` Gustavo A. R. Silva
2020-07-22 22:40       ` Winkler, Tomas
2020-07-22 23:01         ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.