Hi Kees and Christophe,
First of all, thanks for the reviews, comments and advices.
On Mon, May 06, 2024 at 09:23:15AM -0700, Kees Cook wrote:
> 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.
Thanks for this clarification. I will change the type for the next
version.
>
> > [...]
> > > 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.
Thanks for explain things well.
> 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);
> }
>
Ok, I'll do it like this for the next version.
>
>
> 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.
Thanks for letting me know. I will take a look.
>
> --
Again, thanks,
Erick
> Kees Cook
