Re: [PATCH bpf-next] selftests/bpf: Replace %pK output with 0

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-09-11 15:41:45 [+0000], bot+bpf-ci@kernel.org wrote:
> > diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
> > index 07c834433b38..75ff1142c185 100644
> > --- a/tools/testing/selftests/bpf/README.rst
> > +++ b/tools/testing/selftests/bpf/README.rst
> > @@ -187,7 +187,7 @@ The symptom for ``bpf_iter/netlink`` looks like
> >    ...
> >    15: (bf) r2 = r7
> >    16: (0f) r2 += r1
> > -  ; BPF_SEQ_PRINTF(seq, "%pK %-3d ", s, s->sk_protocol);
> > +  ; BPF_SEQ_PRINTF(seq, "%p %-3d ", s, s->sk_protocol);
> >    17: (7b) *(u64 *)(r7 +0) = r2
> >    only read is supported
> 
> This excerpt is a historical clang-10 verifier log illustrating an old
> failure case. The patch rewrites the quoted source to "%p %-3d ", but the
> actual code now reads BPF_SEQ_PRINTF(seq, "0  %-3d ", s->sk_protocol) in
> bpf_iter_netlink.c - the sock pointer argument is gone entirely.
> 
> Should the historical log remain untouched, or should it be updated to
> reflect that the netlink iterator no longer passes a pointer to
> BPF_SEQ_PRINTF?

I would prefer to remove the K and the plain %p should provide the same
historical input. Replacing it with 0 (and removing the argument) might
no longer do the job. As of commit ce3267a39a92b ("kbuild: Bump minimum
version of LLVM for building the kernel to 17.0.1") the minimum required
version is way passed 10. I don't know if this also applies to BPF here
(but why not?). If the required version is 17+, then there is probably
no need to reference a fix for llvm which is included as 11+.

> > diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > index 4e4a82d54f79..f2cd442c4683 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> > @@ -16,7 +16,7 @@
> >  #define MIN_SYM_RET  sizeof(EXP_SYM_OUT)
> >
> >  /* The third specifier, %p, is a hashed pointer which changes on every reboot */
> > -#define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 "
> > +#define EXP_ADDR_OUT "ffff00000add4e55 "
> >  #define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr")
> 
> The comment says %p is the third specifier, but after removing %pK from
> the BPF program (test_snprintf.c:59), %p is now the second specifier in
> the format string "%px %p".
> 
> Should this comment be updated to say "The second specifier"?

Yeah, that makes sense.

> [ ... ]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > index a2652c8c3616..36690478992f 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > @@ -44,8 +44,7 @@ int dump_unix(struct bpf_iter__unix *ctx)
> >  	if (seq_num == 0)
> >  		BPF_SEQ_PRINTF(seq, "Num               RefCount Protocol Flags    Type St    Inode Path\n");
> >
> > -	BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %8lu",
> > -		       unix_sk,
> > +	BPF_SEQ_PRINTF(seq, "%08u: %08X %08X %08X %04X %02X %8lu", 0,
> >  		       sk->sk_refcnt.refs.counter,
> >  		       0,
> >  		       sk->sk_state == TCP_LISTEN ? __SO_ACCEPTCON : 0,
> 
> The header shows "Num               RefCount" (18 columns before RefCount).
> The old format "%pK: " produced 18 columns (16 hex digits + ':' + ' '),
> matching the header width.
> 
> The new format "%08u: " with argument 0 produces "00000000: " (10 columns),
> so every column from RefCount onward is now shifted 8 characters left of
> its header label.
> 
> Should the header be updated to match the new field width, similar to how
> bpf_iter_netlink.c was adjusted from "sk               Eth" to "sk Eth"?

Missed that. The net: version expected a 32bit pointer while the bpf one
expects always a 64bit pointer. I tried to shrink the net one to
something smaller but there was a selftest somewhere expecting the exact
size so I just went with it.

Let me align everything with net then shrink this one including the
description, too.

> Regarding the commit message:
> 
> > Networking recently replaced the socket output with a plain 0 because the
> > socket pointer added no value to the output and the %pK should be removed
> > fom library handling. The format remained otherwise unchanged in order not
> > to break any tools which are parsing this information.
> 
> There's a typo: "fom library handling" should be "from library handling".

Thank you.

> Also, the statement "The format remained otherwise unchanged" is
> contradicted by the netlink header change from "sk               Eth Pid"
> to "sk Eth Pid", and by the field width changes across all modified files
> (varying from 1 to 10 columns vs the original 16-17 columns).
> 
> If maintaining format stability for parsers is the stated goal, should the
> width-preserving spelling be used consistently?

"stability for parsers" means not to remove pointer and therefore a
column so it will work for the human eye but the parse would expect a
certain column. The padding here is just for the human eye and the
parser does not care.

Sebastian