Re: [PATCH] char: hpet: Use flexible-array member

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 1/23/20 12:25, Greg Kroah-Hartman wrote:
> On Mon, Jan 20, 2020 at 05:53:26PM -0600, Gustavo A. R. Silva wrote:
>> Old code in the kernel uses 1-byte and 0-byte arrays to indicate the
>> presence of a "variable length array":
>>
>> struct something {
>>     int length;
>>     u8 data[1];
>> };
>>
>> struct something *instance;
>>
>> instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
>> instance->length = size;
>> memcpy(instance->data, source, size);
>>
>> There is also 0-byte arrays. Both cases pose confusion for things like
>> sizeof(), CONFIG_FORTIFY_SOURCE, etc.[1] Instead, the preferred mechanism
>> to declare variable-length types such as the one above is a flexible array
>> member[2] which need to be the last member of a structure and empty-sized:
>>
>> struct something {
>>         int stuff;
>>         u8 data[];
>> };
>>
>> Also, by making use of the mechanism above, we will get a compiler warning
>> in case the flexible array does not occur last in the structure, which
>> will help us prevent some kind of undefined behavior bugs from being
>> unadvertenly introduced[3] to the codebase from now on.
>>
>> [1] https://github.com/KSPP/linux/issues/21
>> [2] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
>> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/char/hpet.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
>> index 9ac6671bb514..aed2c45f7968 100644
>> --- a/drivers/char/hpet.c
>> +++ b/drivers/char/hpet.c
>> @@ -110,7 +110,7 @@ struct hpets {
>>  	unsigned long hp_delta;
>>  	unsigned int hp_ntimer;
>>  	unsigned int hp_which;
>> -	struct hpet_dev hp_dev[1];
>> +	struct hpet_dev hp_dev[];
> 
> Are you sure the allocation size is the same again?  Much like the
> n_hdlc patch was, I think you need to adjust the variable size here.
> Maybe, it's a bit of a pain to figure out at a quick glance, I just want
> to make sure you at least do look at that :)
> 

Yep. The allocation thing was already handled almost a year ago by the
following patch, and it didn't require to increase the size at that time:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=401c9bd10beef4b030eb9e34d16b5341dc6c683b

Thanks
--
Gustavo