[tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

Posted by tip-bot2 for Thorsten Blum 11 months, 1 week ago

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     077dcef270361089c322a969b792438b33cfb479
Gitweb:        https://git.kernel.org/tip/077dcef270361089c322a969b792438b33cfb479
Author:        Thorsten Blum <thorsten.blum@linux.dev>
AuthorDate:    Tue, 04 Mar 2025 19:30:57 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 04 Mar 2025 19:58:01 +01:00

perf/x86: Annotate struct bts_buffer with __counted_by()

Add the __counted_by() compiler attribute to the flexible array member
buf to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Use struct_size() to calculate the number of bytes to allocate for a new
bts_buffer. Compared to offsetof(), struct_size() has additional
compile-time checks (e.g., __must_be_array()).

No functional changes intended.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lore.kernel.org/r/20250304183056.78920-2-thorsten.blum@linux.dev
---
 arch/x86/events/intel/bts.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 8e09319..debfc18 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -58,7 +58,7 @@ struct bts_buffer {
 	local_t		head;
 	unsigned long	end;
 	void		**data_pages;
-	struct bts_phys	buf[];
+	struct bts_phys	buf[] __counted_by(nr_bufs);
 };
 
 static struct pmu bts_pmu;
@@ -101,7 +101,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
 	if (overwrite && nbuf > 1)
 		return NULL;
 
-	buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
+	buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
 	if (!buf)
 		return NULL;

Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

Posted by Ingo Molnar 11 months, 1 week ago

* tip-bot2 for Thorsten Blum <tip-bot2@linutronix.de> wrote:

> The following commit has been merged into the perf/core branch of tip:
> 
> Commit-ID:     077dcef270361089c322a969b792438b33cfb479
> Gitweb:        https://git.kernel.org/tip/077dcef270361089c322a969b792438b33cfb479
> Author:        Thorsten Blum <thorsten.blum@linux.dev>
> AuthorDate:    Tue, 04 Mar 2025 19:30:57 +01:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> CommitterDate: Tue, 04 Mar 2025 19:58:01 +01:00
> 
> perf/x86: Annotate struct bts_buffer with __counted_by()
> 
> Add the __counted_by() compiler attribute to the flexible array member
> buf to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
> 
> Use struct_size() to calculate the number of bytes to allocate for a new
> bts_buffer. Compared to offsetof(), struct_size() has additional
> compile-time checks (e.g., __must_be_array()).
> 
> No functional changes intended.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Link: https://lore.kernel.org/r/20250304183056.78920-2-thorsten.blum@linux.dev
> ---
>  arch/x86/events/intel/bts.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
> index 8e09319..debfc18 100644
> --- a/arch/x86/events/intel/bts.c
> +++ b/arch/x86/events/intel/bts.c
> @@ -58,7 +58,7 @@ struct bts_buffer {
>  	local_t		head;
>  	unsigned long	end;
>  	void		**data_pages;
> -	struct bts_phys	buf[];
> +	struct bts_phys	buf[] __counted_by(nr_bufs);
>  };
>  
>  static struct pmu bts_pmu;
> @@ -101,7 +101,7 @@ bts_buffer_setup_aux(struct perf_event *event, void **pages,
>  	if (overwrite && nbuf > 1)
>  		return NULL;

Actually, on a second thought:

> -	buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
> +	buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);

Firstly, in what world is 'buf, buf' more readable? One is a member of 
a structure, the other is the name of the structure - and they match, 
which shows that this function's naming conventions are a mess.

Which should be fixed first ...

I'm also not sure the code is correct ...

So I zapped this commit from tip:perf/core.

Thanks,

	Ingo

Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

Posted by Thorsten Blum 11 months, 1 week ago

On 5. Mar 2025, at 10:18, Ingo Molnar wrote:
> Actually, on a second thought:
> 
>> - buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
>> + buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
> 
> Firstly, in what world is 'buf, buf' more readable? One is a member of 
> a structure, the other is the name of the structure - and they match, 
> which shows that this function's naming conventions are a mess.
> 
> Which should be fixed first ...

Yes, I noticed this too, but since buf->buf[] is used all over the place
(also in other functions), I didn't rename it in this patch.

We could just keep offsetof(struct bts_buffer, buf[nbuf]), or use
struct_size_t(struct bts_buffer, buf, nbuf) and still benefit from
additional compile-time checks, or rename the local variable to struct
bts_buffer *bts and use struct_size(bts, buf, nbuf), for example. Any
preferences or other ideas?

> I'm also not sure the code is correct ...

Which part of it?

Thanks,
Thorsten

Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

Posted by Ingo Molnar 11 months, 1 week ago

* Thorsten Blum <thorsten.blum@linux.dev> wrote:

> On 5. Mar 2025, at 10:18, Ingo Molnar wrote:
> > Actually, on a second thought:
> > 
> >> - buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
> >> + buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
> > 
> > Firstly, in what world is 'buf, buf' more readable? One is a member of 
> > a structure, the other is the name of the structure - and they match, 
> > which shows that this function's naming conventions are a mess.
> > 
> > Which should be fixed first ...
> 
> Yes, I noticed this too, but since buf->buf[] is used all over the place
> (also in other functions), I didn't rename it in this patch.
> 
> We could just keep offsetof(struct bts_buffer, buf[nbuf]), or use
> struct_size_t(struct bts_buffer, buf, nbuf) and still benefit from
> additional compile-time checks, or rename the local variable to struct
> bts_buffer *bts and use struct_size(bts, buf, nbuf), for example. Any
> preferences or other ideas?

To clean up this code before changing it, so that the changes become 
obvious to review.

Please also split out the annotation for instrumentation, it's separate 
from any struct_size() changes, right?

> > I'm also not sure the code is correct ...
> 
> Which part of it?

The size calculation. On a second reading I *think* it's correct, but 
it's unnecessarily confusing due to the buf<->buf aliasing.

So in a cleaned up version of the code:

  - If we name 'struct bts_buffer' objects 'bb'
  - and bb:buf[] is the var-array
  - and we rename 'nbuf' to 'nr_buf' (the number of bb:buf[] elements)

then the code right now does:

        bb = kzalloc_node(offsetof(struct bts_buffer, bb[nr_buf]), GFP_KERNEL, node);

... which looks correct.

Thanks,

	Ingo

Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

Posted by Thorsten Blum 11 months, 1 week ago

On 5. Mar 2025, at 12:02, Ingo Molnar wrote:
> * Thorsten Blum <thorsten.blum@linux.dev> wrote:
>> On 5. Mar 2025, at 10:18, Ingo Molnar wrote:
>>> Actually, on a second thought:
>>> 
>>>> - buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
>>>> + buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
>>> 
>>> Firstly, in what world is 'buf, buf' more readable? One is a member of 
>>> a structure, the other is the name of the structure - and they match, 
>>> which shows that this function's naming conventions are a mess.
>>> 
>>> Which should be fixed first ...
>> 
>> Yes, I noticed this too, but since buf->buf[] is used all over the place
>> (also in other functions), I didn't rename it in this patch.
>> 
>> We could just keep offsetof(struct bts_buffer, buf[nbuf]), or use
>> struct_size_t(struct bts_buffer, buf, nbuf) and still benefit from
>> additional compile-time checks, or rename the local variable to struct
>> bts_buffer *bts and use struct_size(bts, buf, nbuf), for example. Any
>> preferences or other ideas?
> 
> To clean up this code before changing it, so that the changes become 
> obvious to review.
> 
> Please also split out the annotation for instrumentation, it's separate 
> from any struct_size() changes, right?

Yes, I'll send a v2 with the __counted_by() annotation and submit a
separate patch for struct_size() and other changes.

>>> I'm also not sure the code is correct ...
>> 
>> Which part of it?
> 
> The size calculation. On a second reading I *think* it's correct, but 
> it's unnecessarily confusing due to the buf<->buf aliasing.
> 
> So in a cleaned up version of the code:
> 
> - If we name 'struct bts_buffer' objects 'bb'
> - and bb:buf[] is the var-array
> - and we rename 'nbuf' to 'nr_buf' (the number of bb:buf[] elements)
> 
> then the code right now does:
> 
>       bb = kzalloc_node(offsetof(struct bts_buffer, bb[nr_buf]), GFP_KERNEL, node);
> 
> ... which looks correct.

If bb:buf[] is the flexible array, it should be buf[nr_buf] like this:

	bb = kzalloc_node(offsetof(struct bts_buffer, buf[nr_buf]), GFP_KERNEL, node);

Thanks,
Thorsten