| Message ID | 20250304183056.78920-2-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | perf/x86: Annotate struct bts_buffer with __counted_by() | expand |
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
* 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
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
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c index 8f78b0c900ef..2888edb3f7c5 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;
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> --- arch/x86/events/intel/bts.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)