Re: [PATCH bpf-next v5 07/14] bpf: Track verifier context diagnostic events
| Newsgroups | org.kernel.vger.bpf |
|---|---|
| Message-ID | <a4998b9ef3040cb6ae37c6fb2ee4444c737531839d530cafbe4657294af40806@mail.kernel.org> |
> bpf: Track verifier context diagnostic events > > Record verifier context transitions in the diagnostic history so later reports > can anchor causal paths to the critical section that made an operation invalid. > > This covers lock, IRQ, RCU, and preempt regions without adding any new > verifier error reports. Category-specific commits decide where those recorded > events should be rendered. > > Use context depth when selecting scoped history so nested regions anchor at the > outer active region, and fall back to the earliest retained event when the > matching entry was pruned. > > Acked-by: Eduard Zingerman <[email protected]> > Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]> The third paragraph describes history-selection behavior that this commit does not implement. Does this paragraph belong in a later commit instead? This patch records BPF_DIAG_HISTORY_CONTEXT events via bpf_diag_record_context(), but nothing in the diff consumes the event log or selects a scoped history range. The code that actually does what the paragraph describes is diag_history_context_start_idx() in kernel/bpf/diagnostics.c: if (!opts->ctx_depth) return 0; /* Find the most recent outermost entry, or a depth-zero exit. */ for (i = log->cnt; i > 0; i--) { ... if (event->ctx.enter && event->ctx.depth == 1) return i - 1; /* anchor at the outer active region */ if (!event->ctx.enter && event->ctx.depth == 0) return 0; /* earliest retained event */ } and it appears in the next commit d08c774626ab ("bpf: Report Register Type Safety errors") together with struct bpf_diag_history_opts::ctx_depth and diag_history_start_idx(). Neither bpf_diag_history_opts nor any scoped-history selector exists at this commit. The recorded depths are correct and match the later consumer. But a reviewer reading this patch will search for the described selection logic and not find it, which conflicts with the second paragraph that correctly states "Category-specific commits decide where those recorded events should be rendered." Could the third paragraph be moved to d08c774626ab, or reworded to say the depth field is recorded here so that later commits can anchor scoped history? --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31870608176