linux-kernel.vger.kernel.org archive mirror
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).