Received-SPF: pass (google.com: domain of linux-kernel+bounces-169083-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 5 May 2024 19:31:24 +0200
From: Erick Archer <erick.archer@outlook.com>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Erick Archer <erick.archer@outlook.com>,
	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>,
	Kees Cook <keescook@chromium.org>,
	"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
Subject: Re: [PATCH v2] perf/ring_buffer: Prefer struct_size over open coded
 arithmetic
Message-ID:
 <AS8PR02MB7237EF9D1962834BE4D2C5F88B1D2@AS8PR02MB7237.eurprd02.prod.outlook.com>
References: <AS8PR02MB7237569E4FBE0B26F62FDFDB8B1D2@AS8PR02MB7237.eurprd02.prod.outlook.com>
 <51a49bae-bd91-428e-b476-f862711453a0@wanadoo.fr>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <51a49bae-bd91-428e-b476-f862711453a0@wanadoo.fr>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: AS8PR02MB7237:EE_|AS8PR02MB9909:EE_
X-MS-Office365-Filtering-Correlation-Id: 411f1f26-1c16-4bfc-22f5-08dc6d293828
X-Microsoft-Antispam:
	BCL:0;ARA:14566002|461199019|440099019|3412199016|56899024|1710799017;
X-Microsoft-Antispam-Message-Info:
	VXkplHxuGEUFJylQiOBQeLyVeRz+3zeYzoew2Ds0TLWHqRYjrV6M1T2XZgXn/TfRjMSpKE6IR5y/wVRjEaNdAVCTPRmuHJA8zDtEvD8/OiL+QgcNlqmqqeObzU6ZzNzw5+fZNuXc5XVIhWD8P1TVvn/IoV1I1q0KTQwCi/cMAiAJQM03WgwqP3/JbzjhuViOj+S71Xeb/Nes1GwQ+F/HgpcZdldTFAaw0NQPvbwzfAeSS7currwD+j4KghjmrCE7l1W4cXnvWQ9CtNimLyPk7TW+LACg1nSLRu+7vO5KcUglkKR5zbpx5S2JzDL5F1BkvA7PQ5Z1hhyyCU3/eou0Ho1/DOlAdedl2G8czODRAhMXFEXfWITZwfWitCW8wZaoj2HvxXIcJt+zfYsNFNyA8rcdd277Q2gZD44x/R84Rpv3s2GhJG6vC/WvenGN+EVffa5kVoalyFJ8u/aIKtAPzOZsh6Yn9uKxJ3DLFUIecYb+/Z4XL6mCxpah2GIqtHjb74vxV/yAOyHd9CbQrlZKfDSua60Pkp2hTntToSl7DuIkmyq5KN69leFaeyQetPOcDtxto/PebBLjD5depnyvDTw0Luk+VFF+1q2kXEIx/jc=
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?iso-8859-1?Q?DlWfybdKFHUzdbcdrbJzPkG0gOOhGRqnEwr7Z1oVkucwr3E4bGfhLyKObw?=
 =?iso-8859-1?Q?of3HSZc3MeOLa/NI2LLsE4uXxaIbWog90r5a8Cwjh3mtLLC8MjUtjizkUo?=
 =?iso-8859-1?Q?p70glhZ/GcPAysUk7RuIOMKfWq/WVwsjd+ApMNeMIfJHyc0zvK5GMOtama?=
 =?iso-8859-1?Q?EcL/Hp7js89gHRvBFojA6fKcLJrIVz71uuZTrt2NIrN/idGgupyAA/+yLh?=
 =?iso-8859-1?Q?j9b1zrQXuLGYxZdWFB60FzbQ3qo09cMNZMfH9wwkflbdCt9SetItT66mwH?=
 =?iso-8859-1?Q?KoCGAY0j0T/HHY76/VPvbAbjJEKgvJjzFtVpV5oe3PZShn7oqUG//xbFxq?=
 =?iso-8859-1?Q?9uaVivtxRX9W0o/G88ZmpCCQ/uIVQb8530zUXD+atM6a4mxHQ1d4LxzT4l?=
 =?iso-8859-1?Q?MKAy4gmMKL+xOR47hNHzoEqIHjgugXI31UTU1RdvtKU9LLot1WHKe9BTl9?=
 =?iso-8859-1?Q?xFH7c1O/rsMQ3H2bxJTQutfNJ5jjABGnd0c1YgUzXRXdscN0VbI7Z/iLJN?=
 =?iso-8859-1?Q?HabTnGsqribVVqgIIlIRPZM5wh8tdBbQgyDcN4RFvSccBue0OKS+XarNjR?=
 =?iso-8859-1?Q?7IGbF8YjxE24yjMcIo6N6enoWRpCjEgeOtqHLvc40vFKG/oAFpYKTUrhh+?=
 =?iso-8859-1?Q?lHcPyz2gQ5v0UdvNlHkMetrrjbo9Fxur6kZ5vW6ONlrkPn6akU6v6Mlwn3?=
 =?iso-8859-1?Q?V82zDKdc7bXmPOz3qDqJVho+7kcyDpxHJHPolSJosDikiCIrwn5NMfObNh?=
 =?iso-8859-1?Q?1QQHZ5hnq7pKDExAHAduGTw1Wap1VsPO+eYygAgdbvGDBfAsM8rGhmwZFz?=
 =?iso-8859-1?Q?Y//JdGZV4Gh0kYMXfVC7aPhJHZgo0N4hmOKXd7Egcy6vaQlvDxqfaC6SiD?=
 =?iso-8859-1?Q?WoJyYzbzVHUWQXajQh/A3l4Lu2k1lMq2Smi1wA3mt5fUkWF2+CatT0fMYs?=
 =?iso-8859-1?Q?g2w2tC5+lxuNQD5XvMu5X6xNo9/34YCiyMVGH6Jt0ZuA5ADXQhHAnzF0jk?=
 =?iso-8859-1?Q?opr8IkHG0UFJnNniyxEVynmIoK4p4Ge/mFor0hiATnibSqQaahNANdtITf?=
 =?iso-8859-1?Q?2ggYqJAnxy3FH9ZfAiMlq0Abg9h3cioEb0voH7pcWy97DTdRnw1O9fs3eZ?=
 =?iso-8859-1?Q?nWXp7F54/2fGyrkUF21m24duB1GGlPbGBpQGHLML6nw/+TRKl12WPR9PcH?=
 =?iso-8859-1?Q?VpSGnBo3kpMpKxA1buSEMMBqfaWRTRD/ONZIyeZFVklQQJluPDpQNnz23o?=
 =?iso-8859-1?Q?ZXgS8tZN8yWbDFl+bpP43/MeZcpyU9WIKCslYVkIZ+DuDRcs9Zlm3EEHNO?=
 =?iso-8859-1?Q?AsAA?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 411f1f26-1c16-4bfc-22f5-08dc6d293828
X-MS-Exchange-CrossTenant-AuthSource: AS8PR02MB7237.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2024 17:31:38.5531
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR02MB9909

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.

The size type for the kzalloc_node function is "size_t" but for the
order_base_2() macro it is necessary an unsigned type (since this
is expanded to "__ilog2_u32(u32 n)" or "__ilog2_u64(u64 n)").

So, I don't know if it is correct to change the type to size_t.
Maybe someone can help with this.

> 
> >   	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;
> > @@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
> >   	if (!rb)
> >   		goto fail;
> > +	rb->nr_pages = nr_pages;
> >   	rb->user_page = perf_mmap_alloc_page(cpu);
> >   	if (!rb->user_page)
> >   		goto fail_user_page;
> > @@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
> >   			goto fail_data_pages;
> >   	}
> > -	rb->nr_pages = nr_pages;
> > -
> >   	ring_buffer_init(rb, watermark, flags);
> >   	return rb;
> > @@ -916,18 +913,15 @@ 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;
> > +	rb->nr_pages = nr_pages;
> 
> I don't think this is correct.

I think you are right. My bad :(

> There is already a logic in place about it a few lines below:
> 
> 	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 what is needed is to move this block just 2 lines above,
> (before rb->data_pages[0] = ...)
> 
> 
> I'm also wondering what should be done if nr_pages = 0.

Perhaps this is enough since we only allocate memory for one
member of the array.

@@ -916,18 +913,15 @@ 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;

+       rb->nr_pages = 1;
        INIT_WORK(&rb->work, rb_free_work);

        all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);

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?

Regards,
Erick

> CJ
> 
> >   	INIT_WORK(&rb->work, rb_free_work);
> >   	all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
>