On Sun, May 05, 2024 at 07:31:24PM +0200, Erick Archer wrote: > On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote: > > Le 05/05/2024 à 16:15, Erick Archer a écrit : > > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > > > index 4013408ce012..080537eff69f 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; > > > > Hi, > > > > Should size be size_t? > > I'm sorry, but I don't have enough knowledge to answer this question. > The "size" variable is used as a return value by struct_size and as > a parameter to the order_base_2() and kzalloc_node() functions.
For Linux, size_t and unsigned long are the same (currently). Pedantically, yes, this should be size_t, but it's the same. > [...] > > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE); > > if (!all_buf) > > goto fail_all_buf; > > > > rb->user_page = all_buf; > > rb->data_pages[0] = all_buf + PAGE_SIZE; > > if (nr_pages) { <--- here > > rb->nr_pages = 1; <--- > > rb->page_order = ilog2(nr_pages); > > } > [...] > I think that we don't need to deal with the "nr_pages = 0" case > since the flex array will always have a length of one. > > Kees, can you help us with this? Agh, this code hurt my head for a while. all_buf contains "nr_pages + 1" pages. all_buf gets attached to rb->user_page, and then rb->data_pages[0] points to the second page in all_buf... which means, I guess, that rb->data_pages does only have 1 entry. However, the nr_pages == 0 case is weird. Currently, data_pages[0] will still get set (which points ... off the end of all_buf). If we unconditionally set rb->nr_pages to 1, we're changing the behavior. If we _don't_ set rb->data_pages[0], we're changing the behavior, but I think it's an invalid pointer anyway, so this is the safer change to make. I suspect the right replacement is: diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c index 4013408ce012..7d638ce76799 100644 --- a/kernel/events/ring_buffer.c +++ b/kernel/events/ring_buffer.c @@ -916,15 +916,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, nr_pages, 1), GFP_KERNEL, node); if (!rb) goto fail; @@ -935,9 +931,9 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) goto fail_all_buf; rb->user_page = all_buf; - rb->data_pages[0] = all_buf + PAGE_SIZE; if (nr_pages) { rb->nr_pages = 1; + rb->data_pages[0] = all_buf + PAGE_SIZE; rb->page_order = ilog2(nr_pages); } Also, why does rb_alloc() take an "int" nr_pages? The only caller has an unsigned long argument for nr_pages. Nothing checks for >INT_MAX that I can find. -- Kees Cook