| Message ID | AS8PR02MB72372AB065EA8340D960CCC48B1B2@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | perf/ring_buffer: Prefer struct_size over open coded arithmetic | expand |
On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > As the "rb" variable is a pointer to "struct perf_buffer" and this > structure ends in a flexible array: > > struct perf_buffer { > [...] int nr_pages; /* nr of data pages */ ... > void *data_pages[]; > }; This should gain __counted_by(nr_pages) with a little refactoring. > > the preferred way in the kernel is to use the struct_size() helper to > do the arithmetic instead of the calculation "size + count * size" in > the kzalloc_node() functions. > > In the "rb_alloc" function defined in the else branch of the macro > > #ifndef CONFIG_PERF_USE_VMALLOC > > the count in the struct_size helper is the literal "1" since only one > pointer to void is allocated. Also, remove the "size" variable as it > is no longer needed. > > This way, the code is more readable and safer. > > This code was detected with the help of Coccinelle, and audited and > modified manually. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/160 [2] > Signed-off-by: Erick Archer <erick.archer@outlook.com> > --- > Hi, > > The Coccinelle script used to detect this code pattern is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) > ... > i0 += sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > Regards, > Erick > --- > kernel/events/ring_buffer.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index 4013408ce012..e68b02a56382 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > unsigned long size; > int i, node; > > - size = sizeof(struct perf_buffer); > - size += nr_pages * sizeof(void *); > - > + size = struct_size(rb, data_pages, nr_pages); > if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER) > goto fail; This looks good. Continuing... node = (cpu == -1) ? cpu : cpu_to_node(cpu); rb = kzalloc_node(size, GFP_KERNEL, node); if (!rb) goto fail; then move this line up from below the array-writing loop: rb->nr_pages = nr_pages; > > @@ -916,15 +914,11 @@ void rb_free(struct perf_buffer *rb) > struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > { > struct perf_buffer *rb; > - unsigned long size; > void *all_buf; > int node; > > - size = sizeof(struct perf_buffer); > - size += sizeof(void *); > - > node = (cpu == -1) ? cpu : cpu_to_node(cpu); > - rb = kzalloc_node(size, GFP_KERNEL, node); > + rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node); > if (!rb) > goto fail; Same move here: rb->nr_pages = nr_pages; Otherwise, yes, looks good! -Kees
On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. So personally I detest struct_size() because I can never remember wtf it does, whereas the code it replaces is simple and straight forward :/
On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > This is an effort to get rid of all multiplications from allocation > > functions in order to prevent integer overflows [1][2]. > > So personally I detest struct_size() because I can never remember wtf it > does, whereas the code it replaces is simple and straight forward :/ Ok, I accept what you say. Anyway, I think it's a matter of preference ;) If I prepare a patch with the changes suggested by Kees to gain coverage of the __counted_by attribute, will it have a chance of being accepted? Are there any guidelines related to this attribute, such as in the case of the struct_size() helper? Regards, Erick
On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > This is an effort to get rid of all multiplications from allocation > > functions in order to prevent integer overflows [1][2]. > > So personally I detest struct_size() because I can never remember wtf it > does, whereas the code it replaces is simple and straight forward :/ Sure, new APIs can involved a learning curve. If we can all handle container_of(), we can deal with struct_size(). :)
On Wed, May 01, 2024 at 01:21:42PM -0700, Kees Cook wrote: > On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > > This is an effort to get rid of all multiplications from allocation > > > functions in order to prevent integer overflows [1][2]. > > > > So personally I detest struct_size() because I can never remember wtf it > > does, whereas the code it replaces is simple and straight forward :/ > > Sure, new APIs can involved a learning curve. If we can all handle > container_of(), we can deal with struct_size(). :) containre_of() is actually *much* shorter than typing it all out. Which is a benefit. struct_size() not so much. That's just obfuscation for obfuscation's sake.
On Thu, May 02, 2024 at 11:18:37AM +0200, Peter Zijlstra wrote: > On Wed, May 01, 2024 at 01:21:42PM -0700, Kees Cook wrote: > > On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > > > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > > > This is an effort to get rid of all multiplications from allocation > > > > functions in order to prevent integer overflows [1][2]. > > > > > > So personally I detest struct_size() because I can never remember wtf it > > > does, whereas the code it replaces is simple and straight forward :/ > > > > Sure, new APIs can involved a learning curve. If we can all handle > > container_of(), we can deal with struct_size(). :) > > containre_of() is actually *much* shorter than typing it all out. Which > is a benefit. > > struct_size() not so much. That's just obfuscation for obfuscation's > sake. It's really not -- it's making sure that the calculation is semantically sane: all the right things are being used for the struct size calculation and things can't "drift", if types change, flex array changes, etc. It's both a code robustness improvement and a wrap-around stopping improvement.
On Thu, May 02, 2024 at 03:55:36PM -0700, Kees Cook wrote: > On Thu, May 02, 2024 at 11:18:37AM +0200, Peter Zijlstra wrote: > > On Wed, May 01, 2024 at 01:21:42PM -0700, Kees Cook wrote: > > > On Tue, Apr 30, 2024 at 11:15:04AM +0200, Peter Zijlstra wrote: > > > > On Mon, Apr 29, 2024 at 07:40:58PM +0200, Erick Archer wrote: > > > > > This is an effort to get rid of all multiplications from allocation > > > > > functions in order to prevent integer overflows [1][2]. > > > > > > > > So personally I detest struct_size() because I can never remember wtf it > > > > does, whereas the code it replaces is simple and straight forward :/ > > > > > > Sure, new APIs can involved a learning curve. If we can all handle > > > container_of(), we can deal with struct_size(). :) > > > > containre_of() is actually *much* shorter than typing it all out. Which > > is a benefit. > > > > struct_size() not so much. That's just obfuscation for obfuscation's > > sake. I do not agree with this. > > It's really not -- it's making sure that the calculation is semantically > sane: all the right things are being used for the struct size calculation > and things can't "drift", if types change, flex array changes, etc. It's > both a code robustness improvement and a wrap-around stopping improvement. > Also, in the "Deprecated Interfaces, Language Features, Attributes, and Conventions" [1] it says verbatim: Another common case to avoid is calculating the size of a structure with a trailing array of others structures, as in: header = kzalloc(sizeof(*header) + count * sizeof(*header->item), GFP_KERNEL); Instead, use the helper: header = kzalloc(struct_size(header, item, count), GFP_KERNEL); Therefore, if there is a convention to follow, we should not make an exception. Moreover, struct_size is widely used in the kernel and widely accepted. Also makes the code safer. So, I will send a new patch with the changes Kees proposed and I hope that it will be the first step in the adoption of struct_size in the perf and sched subsystems ;) Regards, Erick [1] https://docs.kernel.org/process/deprecated.html > -- > Kees Cook
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c index 4013408ce012..e68b02a56382 100644 --- a/kernel/events/ring_buffer.c +++ b/kernel/events/ring_buffer.c @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) unsigned long size; int i, node; - size = sizeof(struct perf_buffer); - size += nr_pages * sizeof(void *); - + size = struct_size(rb, data_pages, nr_pages); if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER) goto fail; @@ -916,15 +914,11 @@ void rb_free(struct perf_buffer *rb) struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) { struct perf_buffer *rb; - unsigned long size; void *all_buf; int node; - size = sizeof(struct perf_buffer); - size += sizeof(void *); - node = (cpu == -1) ? cpu : cpu_to_node(cpu); - rb = kzalloc_node(size, GFP_KERNEL, node); + rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node); if (!rb) goto fail;
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. As the "rb" variable is a pointer to "struct perf_buffer" and this structure ends in a flexible array: struct perf_buffer { [...] void *data_pages[]; }; the preferred way in the kernel is to use the struct_size() helper to do the arithmetic instead of the calculation "size + count * size" in the kzalloc_node() functions. In the "rb_alloc" function defined in the else branch of the macro #ifndef CONFIG_PERF_USE_VMALLOC the count in the struct_size helper is the literal "1" since only one pointer to void is allocated. Also, remove the "size" variable as it is no longer needed. This way, the code is more readable and safer. This code was detected with the help of Coccinelle, and audited and modified manually. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/160 [2] Signed-off-by: Erick Archer <erick.archer@outlook.com> --- Hi, The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) ... i0 += sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) Regards, Erick --- kernel/events/ring_buffer.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)