* [PATCH] tty: n_hdlc: Use flexible-array member
@ 2020-01-20 23:45 Gustavo A. R. Silva
2020-01-21 5:54 ` Jiri Slaby
0 siblings, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2020-01-20 23:45 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-kernel, Gustavo A. R. Silva
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/tty/n_hdlc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 98361acd3053..b5499ca8757e 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -115,7 +115,7 @@
struct n_hdlc_buf {
struct list_head list_item;
int count;
- char buf[1];
+ char buf[];
};
#define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
--
2.23.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-20 23:45 [PATCH] tty: n_hdlc: Use flexible-array member Gustavo A. R. Silva
@ 2020-01-21 5:54 ` Jiri Slaby
2020-01-21 14:27 ` Gustavo A. R. Silva
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Slaby @ 2020-01-21 5:54 UTC (permalink / raw)
To: Gustavo A. R. Silva, Greg Kroah-Hartman; +Cc: linux-kernel
On 21. 01. 20, 0:45, 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/tty/n_hdlc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
> index 98361acd3053..b5499ca8757e 100644
> --- a/drivers/tty/n_hdlc.c
> +++ b/drivers/tty/n_hdlc.c
> @@ -115,7 +115,7 @@
> struct n_hdlc_buf {
> struct list_head list_item;
> int count;
> - char buf[1];
> + char buf[];
> };
>
> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
Have you checked, that you don't have to "+ 1" here now?
Other than that:
Acked-by: Jiri Slaby <jslaby@suse.cz>
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-21 5:54 ` Jiri Slaby
@ 2020-01-21 14:27 ` Gustavo A. R. Silva
2020-01-21 15:00 ` Mikael Magnusson
0 siblings, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2020-01-21 14:27 UTC (permalink / raw)
To: Jiri Slaby, Greg Kroah-Hartman; +Cc: linux-kernel
On 1/20/20 23:54, Jiri Slaby wrote:
> On 21. 01. 20, 0:45, 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/tty/n_hdlc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
>> index 98361acd3053..b5499ca8757e 100644
>> --- a/drivers/tty/n_hdlc.c
>> +++ b/drivers/tty/n_hdlc.c
>> @@ -115,7 +115,7 @@
>> struct n_hdlc_buf {
>> struct list_head list_item;
>> int count;
>> - char buf[1];
>> + char buf[];
>> };
>>
>> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
>
> Have you checked, that you don't have to "+ 1" here now?
>
Yep. That's not necessary.
_In terms of memory allocation_, zero-length/one-element arrays and flexible-array
members work exactly the same way.
> Other than that:
> Acked-by: Jiri Slaby <jslaby@suse.cz>
>
Thanks!
--
Gustavo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-21 14:27 ` Gustavo A. R. Silva
@ 2020-01-21 15:00 ` Mikael Magnusson
2020-01-21 15:14 ` Gustavo A. R. Silva
0 siblings, 1 reply; 7+ messages in thread
From: Mikael Magnusson @ 2020-01-21 15:00 UTC (permalink / raw)
To: gustavo; +Cc: gregkh, jslaby, linux-kernel
Gustavo Silva wrote:
> On 1/20/20 23:54, Jiri Slaby wrote:
>> On 21. 01. 20, 0:45, Gustavo A. R. Silva wrote:
>>> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
>>> index 98361acd3053..b5499ca8757e 100644
>>> --- a/drivers/tty/n_hdlc.c
>>> +++ b/drivers/tty/n_hdlc.c
>>> @@ -115,7 +115,7 @@
>>> struct n_hdlc_buf {
>>> struct list_head list_item;
>>> int count;
>>> - char buf[1];
>>> + char buf[];
>>> };
>>>
>>> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
>>
>> Have you checked, that you don't have to "+ 1" here now?
>>
>
> Yep. That's not necessary.
>
> _In terms of memory allocation_, zero-length/one-element arrays and flexible-array
> members work exactly the same way.
This is not true, but maybe it's still not necessary in this particular code, I didn't examine it.
Consider the following:
#include <stdio.h>
struct flex {
int count;
char buf[PRE];
char flex[];
};
struct one {
int count;
char buf[PRE];
char one[1];
};
void main() {
printf("%ld %ld\n", sizeof(struct flex), sizeof(struct one));
}
--snip--
% gcc -o siz siz.c -std=c99 -DPRE=7 && ./siz
12 12
% gcc -o siz siz.c -std=c99 -DPRE=8 && ./siz
12 16
Since all the preceding stuff in the struct in the patch is aligned, then the [1] will definitely add something to the sizeof count.
--
Mikael Magnusson
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-21 15:00 ` Mikael Magnusson
@ 2020-01-21 15:14 ` Gustavo A. R. Silva
2020-01-21 15:24 ` Gustavo A. R. Silva
0 siblings, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2020-01-21 15:14 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: gregkh, jslaby, linux-kernel
On 1/21/20 09:00, Mikael Magnusson wrote:
> Gustavo Silva wrote:
>> On 1/20/20 23:54, Jiri Slaby wrote:
>>> On 21. 01. 20, 0:45, Gustavo A. R. Silva wrote:
>>>> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
>>>> index 98361acd3053..b5499ca8757e 100644
>>>> --- a/drivers/tty/n_hdlc.c
>>>> +++ b/drivers/tty/n_hdlc.c
>>>> @@ -115,7 +115,7 @@
>>>> struct n_hdlc_buf {
>>>> struct list_head list_item;
>>>> int count;
>>>> - char buf[1];
>>>> + char buf[];
>>>> };
>>>>
>>>> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
>>>
>>> Have you checked, that you don't have to "+ 1" here now?
>>>
>>
>> Yep. That's not necessary.
>>
>> _In terms of memory allocation_, zero-length/one-element arrays and flexible-array
>> members work exactly the same way.
>
> This is not true, but maybe it's still not necessary in this particular code, I didn't examine it.
>
I should have said _in terms of dynamic memory allocation_.
Your example is correct:
"... a one-element array always occupies at least as much space as a single object of the type."[1]
But the above does not affect on the current code.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
Thanks
--
Gustavo
> Consider the following:
> #include <stdio.h>
>
> struct flex {
> int count;
> char buf[PRE];
> char flex[];
> };
>
> struct one {
> int count;
> char buf[PRE];
> char one[1];
> };
>
> void main() {
> printf("%ld %ld\n", sizeof(struct flex), sizeof(struct one));
> }
>
> --snip--
>
> % gcc -o siz siz.c -std=c99 -DPRE=7 && ./siz
> 12 12
> % gcc -o siz siz.c -std=c99 -DPRE=8 && ./siz
> 12 16
>
> Since all the preceding stuff in the struct in the patch is aligned, then the [1] will definitely add something to the sizeof count.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-21 15:14 ` Gustavo A. R. Silva
@ 2020-01-21 15:24 ` Gustavo A. R. Silva
2020-01-21 16:03 ` Gustavo A. R. Silva
0 siblings, 1 reply; 7+ messages in thread
From: Gustavo A. R. Silva @ 2020-01-21 15:24 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: gregkh, jslaby, linux-kernel
On 1/21/20 09:14, Gustavo A. R. Silva wrote:
>
>
> On 1/21/20 09:00, Mikael Magnusson wrote:
>> Gustavo Silva wrote:
>>> On 1/20/20 23:54, Jiri Slaby wrote:
>>>> On 21. 01. 20, 0:45, Gustavo A. R. Silva wrote:
>>>>> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
>>>>> index 98361acd3053..b5499ca8757e 100644
>>>>> --- a/drivers/tty/n_hdlc.c
>>>>> +++ b/drivers/tty/n_hdlc.c
>>>>> @@ -115,7 +115,7 @@
>>>>> struct n_hdlc_buf {
>>>>> struct list_head list_item;
>>>>> int count;
>>>>> - char buf[1];
>>>>> + char buf[];
>>>>> };
>>>>>
>>>>> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
>>>>
>>>> Have you checked, that you don't have to "+ 1" here now?
>>>>
>>>
>>> Yep. That's not necessary.
>>>
>>> _In terms of memory allocation_, zero-length/one-element arrays and flexible-array
>>> members work exactly the same way.
>>
>> This is not true, but maybe it's still not necessary in this particular code, I didn't examine it.
>>
>
> I should have said _in terms of dynamic memory allocation_.
>
> Your example is correct:
>
> "... a one-element array always occupies at least as much space as a single object of the type."[1]
>
> But the above does not affect on the current code.
>
Oh wait! Yeah; I see the issue in #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe) now...
I need to double check this.
Thanks for the feedback!
--
Gustavo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: n_hdlc: Use flexible-array member
2020-01-21 15:24 ` Gustavo A. R. Silva
@ 2020-01-21 16:03 ` Gustavo A. R. Silva
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo A. R. Silva @ 2020-01-21 16:03 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: gregkh, jslaby, linux-kernel
On 1/21/20 09:24, Gustavo A. R. Silva wrote:
>
>
> On 1/21/20 09:14, Gustavo A. R. Silva wrote:
>>
>>
>> On 1/21/20 09:00, Mikael Magnusson wrote:
>>> Gustavo Silva wrote:
>>>> On 1/20/20 23:54, Jiri Slaby wrote:
>>>>> On 21. 01. 20, 0:45, Gustavo A. R. Silva wrote:
>>>>>> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
>>>>>> index 98361acd3053..b5499ca8757e 100644
>>>>>> --- a/drivers/tty/n_hdlc.c
>>>>>> +++ b/drivers/tty/n_hdlc.c
>>>>>> @@ -115,7 +115,7 @@
>>>>>> struct n_hdlc_buf {
>>>>>> struct list_head list_item;
>>>>>> int count;
>>>>>> - char buf[1];
>>>>>> + char buf[];
>>>>>> };
>>>>>>
>>>>>> #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
>>>>>
>>>>> Have you checked, that you don't have to "+ 1" here now?
>>>>>
>>>>
>>>> Yep. That's not necessary.
>>>>
>>>> _In terms of memory allocation_, zero-length/one-element arrays and flexible-array
>>>> members work exactly the same way.
>>>
>>> This is not true, but maybe it's still not necessary in this particular code, I didn't examine it.
>>>
>>
>> I should have said _in terms of dynamic memory allocation_.
>>
>> Your example is correct:
>>
>> "... a one-element array always occupies at least as much space as a single object of the type."[1]
>>
>> But the above does not affect on the current code.
>>
>
> Oh wait! Yeah; I see the issue in #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe) now...
>
This would be the new patch; and I'm making use the the struct_size helper
this time, to safely calculate the allocation size:
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 98361acd3053..27b506bf03ce 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -115,11 +115,9 @@
struct n_hdlc_buf {
struct list_head list_item;
int count;
- char buf[1];
+ char buf[];
};
-#define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
-
struct n_hdlc_buf_list {
struct list_head list;
int count;
@@ -524,7 +522,8 @@ static void n_hdlc_tty_receive(struct tty_struct *tty, const __u8 *data,
/* no buffers in free list, attempt to allocate another rx buffer */
/* unless the maximum count has been reached */
if (n_hdlc->rx_buf_list.count < MAX_RX_BUF_COUNT)
- buf = kmalloc(N_HDLC_BUF_SIZE, GFP_ATOMIC);
+ buf = kmalloc(struct_size(buf, buf, maxframe),
+ GFP_ATOMIC);
}
if (!buf) {
@@ -853,7 +852,7 @@ static struct n_hdlc *n_hdlc_alloc(void)
/* allocate free rx buffer list */
for(i=0;i<DEFAULT_RX_BUF_COUNT;i++) {
- buf = kmalloc(N_HDLC_BUF_SIZE, GFP_KERNEL);
+ buf = kmalloc(struct_size(buf, buf, maxframe), GFP_KERNEL);
if (buf)
n_hdlc_buf_put(&n_hdlc->rx_free_buf_list,buf);
else if (debuglevel >= DEBUG_LEVEL_INFO)
@@ -862,7 +861,7 @@ static struct n_hdlc *n_hdlc_alloc(void)
/* allocate free tx buffer list */
for(i=0;i<DEFAULT_TX_BUF_COUNT;i++) {
- buf = kmalloc(N_HDLC_BUF_SIZE, GFP_KERNEL);
+ buf = kmalloc(struct_size(buf, buf, maxframe), GFP_KERNEL);
if (buf)
n_hdlc_buf_put(&n_hdlc->tx_free_buf_list,buf);
else if (debuglevel >= DEBUG_LEVEL_INFO)
And it seems that that extra "+ 1" is not needed. The frame size is always being
verified in multiple places:
/* verify frame size */
if (count > maxframe ) {
if (debuglevel & DEBUG_LEVEL_INFO)
printk (KERN_WARNING
"n_hdlc_tty_write: truncating user packet "
"from %lu to %d\n", (unsigned long) count,
maxframe );
count = maxframe;
}
^^^^^^
and _count_ is being limited to _maxframe_, before copying data into _buf_ :
/* copy received data to HDLC buffer */
memcpy(buf->buf,data,count);
buf->count=count;
So we might save some bytes, too.
I'll properly write and send v2, shortly.
Thanks
--
Gustavo
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-21 16:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 23:45 [PATCH] tty: n_hdlc: Use flexible-array member Gustavo A. R. Silva
2020-01-21 5:54 ` Jiri Slaby
2020-01-21 14:27 ` Gustavo A. R. Silva
2020-01-21 15:00 ` Mikael Magnusson
2020-01-21 15:14 ` Gustavo A. R. Silva
2020-01-21 15:24 ` Gustavo A. R. Silva
2020-01-21 16:03 ` 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.