Re: [PATCH bpf-next v4 02/16] bpf: Add source and instruction diagnostic context

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2026-08-13 at 01:33 +0200, Kumar Kartikeya Dwivedi wrote:

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

...

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4a10c9878cf..7aad3bbed412 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -4147,8 +4147,17 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
>  }
>  
>  const struct bpf_line_info *bpf_find_linfo(const struct bpf_prog *prog, u32 insn_off);
> -void bpf_get_linfo_file_line(struct btf *btf, const struct bpf_line_info *linfo,
> -			     const char **filep, const char **linep, int *nump);
> +#define BPF_LINFO_LINE_TRIM 1

Nit: a single callsite requires trim, might as well trim there and avoid the flag.

> +struct bpf_linfo_source {
> +	const char *file;
> +	const char *line;
> +	u32 file_name_off;
> +	int line_num;
> +	int line_col;
> +};
> +
> +void bpf_get_linfo_source(struct btf *btf, const struct bpf_line_info *linfo,
> +			  struct bpf_linfo_source *src, u32 flags);

...

> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -214,6 +214,7 @@ int btf_type_seq_show_flags(const struct btf *btf, u32 type_id, void *obj,
>   */
>  int btf_type_snprintf_show(const struct btf *btf, u32 type_id, void *obj,
>  			   char *buf, int len, u64 flags);
> +int btf_type_snprintf_show_name(const struct btf *btf, u32 type_id, char *buf, int len);

Nit: the name of the function suggests that it does a printf.

...

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> index ba57aabd399f..77dcffb9adee 100644
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c

...

> @@ -16,6 +24,50 @@
>  #define POLICY "Policy"
>  #define VERIFIER_LIMIT "Verifier Limit"
>  
> +#define BPF_DIAG_MSG_LEN 512
...
> +#define BPF_DIAG_REG_DESC_LEN 512
...
> +#define BPF_DIAG_REG_TMP_LEN 192

dead constants

...

> +static struct bpf_diag *diag_env(struct bpf_verifier_env *env)
> +{
> +	return env->diag;
> +}

this wrapper is unnecessary

> +static char *diag_fmt_alloc(struct bpf_verifier_env *env, size_t size)
> +{
> +	struct bpf_diag *diag = diag_env(env);
> +	struct diag_fmt_chunk *chunk;
> +	size_t capacity, available;
> +	char *buf;
> +
> +	if (!diag || !size || size > INT_MAX)
> +		return NULL;
> +
> +	if (!list_empty(&diag->fmt_chunks)) {
> +		chunk = list_last_entry(&diag->fmt_chunks, struct diag_fmt_chunk, node);
> +		available = seq_buf_get_buf(&chunk->seq, &buf);
> +		if (available >= size)
> +			goto commit;
> +	}
> +
> +	capacity = max_t(size_t, BPF_DIAG_FMT_CHUNK_SIZE, size);
> +	chunk = kmalloc(struct_size(chunk, data, capacity), GFP_KERNEL_ACCOUNT);

Q: would it be cheaper to allocate one page per chunk?

> +	if (!chunk)
> +		return NULL;
> +
> +	seq_buf_init(&chunk->seq, chunk->data, capacity);
> +	list_add_tail(&chunk->node, &diag->fmt_chunks);
> +	available = seq_buf_get_buf(&chunk->seq, &buf);
> +	if (WARN_ON_ONCE(available < size))
> +		return NULL;
> +
> +commit:
> +	seq_buf_commit(&chunk->seq, size);
> +	return buf;
> +}

...

> +static void diag_fmt_restore(struct bpf_verifier_env *env, struct diag_fmt_mark mark)
> +{
> +	struct bpf_diag *diag = diag_env(env);
> +	struct diag_fmt_chunk *chunk;
> +
> +	if (!diag)
> +		return;
> +
> +	while (!list_empty(&diag->fmt_chunks)) {
> +		chunk = list_last_entry(&diag->fmt_chunks, struct diag_fmt_chunk, node);
> +		if (chunk == mark.chunk)
> +			break;
> +		list_del(&chunk->node);
> +		kfree(chunk);
> +	}
> +
> +	if (mark.chunk) {
> +		mark.chunk->seq.len = mark.len;
> +		seq_buf_str(&mark.chunk->seq);
> +	}
> +}
> +
> +static void diag_fmt_free(struct bpf_verifier_env *env)

Nit: single user, might as well inline. Also, can it be:

     diag_fmt_restore(env, (struct diag_fmt_mark mark){})

     ?

> +{
> +	struct bpf_diag *diag = diag_env(env);
> +	struct diag_fmt_chunk *chunk, *tmp;
> +
> +	if (!diag)
> +		return;
> +
> +	list_for_each_entry_safe(chunk, tmp, &diag->fmt_chunks, node) {
> +		list_del(&chunk->node);
> +		kfree(chunk);
> +	}
> +}
> +
> +void bpf_diag_free(struct bpf_verifier_env *env)
> +{
> +	struct bpf_diag *diag = env->diag;
> +
> +	if (!diag)
> +		return;
> +
> +	diag_fmt_free(env);
> +	kfree(diag);
> +	env->diag = NULL;
> +}
> +

...

> +static void diag_format_source_lane(char *buf, size_t size, const char *source_prefix,
> +				    int source_line_width, int line_num, const char *line)
> +{
> +	int len, text_width;
> +
> +	if (line_num <= 0) {
> +		buf[0] = '\0';
> +		return;
> +	}
> +
> +	len = scnprintf(buf, size, "%s%*d | ", source_prefix, source_line_width, line_num);
> +	if (len >= (int)size)

Can this condition ever be true? scnprintf returns a value in range [0..size].

> +		return;
> +
> +	text_width = BPF_DIAG_SOURCE_LANE_WIDTH - len;
> +	diag_format_source_text(buf + len, size - len, line, text_width);
> +}

...

> +void bpf_diag_source(struct bpf_verifier_env *env, u32 insn_idx, const char *label,
> +			    const char *fmt, ...)
> +{

...

> +	linfo = bpf_find_linfo(env->prog, insn_idx);
> +	if (!btf || !linfo) {
> +		diag_write(env, "  insn %u\n", insn_idx);
> +		diag_print_source_annotation(env, 0, 0, label, msg);
> +		goto out_restore;
> +	}
> +	bpf_get_linfo_source(btf, linfo, &src, 0);
> +	if (!src.file || !*src.file || !src.line || !*src.line) {
> +		diag_write(env, "  insn %u\n", insn_idx);
> +		diag_print_source_annotation(env, 0, 0, label, msg);

Should this case still print instruction context?
(and the one above it).

> +		goto out_restore;
> +	}
> +

...

> diff --git a/kernel/bpf/diagnostics.h b/kernel/bpf/diagnostics.h
> index e8e4c06233e2..7b391cf49ae5 100644

...

> +void bpf_diag_source(struct bpf_verifier_env *env, u32 insn_idx, const char *label,
> +			    const char *fmt, ...) __printf(4, 5);

Nit: no external users.

...