* [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 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).