Received-SPF: pass (google.com: domain of linux-kernel+bounces-170314-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <b3509a36-1fbc-4311-853b-bb5e6d25f0ad@wanadoo.fr>
Date: Mon, 6 May 2024 20:23:43 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] perf/ring_buffer: Prefer struct_size over open coded
 arithmetic
To: Kees Cook <keescook@chromium.org>, Erick Archer <erick.archer@outlook.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
 Adrian Hunter <adrian.hunter@intel.com>,
 "Liang, Kan" <kan.liang@linux.intel.com>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>,
 Nathan Chancellor <nathan@kernel.org>,
 Nick Desaulniers <ndesaulniers@google.com>, Bill Wendling
 <morbo@google.com>, Justin Stitt <justinstitt@google.com>,
 linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org, llvm@lists.linux.dev
References: <AS8PR02MB7237569E4FBE0B26F62FDFDB8B1D2@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <51a49bae-bd91-428e-b476-f862711453a0@wanadoo.fr>
 <AS8PR02MB7237EF9D1962834BE4D2C5F88B1D2@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <202405060856.53AFAE4F22@keescook>
Content-Language: en-MW
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <202405060856.53AFAE4F22@keescook>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Le 06/05/2024 à 18:23, Kees Cook a écrit :
> 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);
>   	}

This is also what make the most sense to me.

CJ

>   
> 
> 
> 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.
>