* Re: [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr
[not found] ` <20250703204818.925464-9-memxor@gmail.com>
@ 2025-07-30 23:02 ` Kees Cook
2025-07-30 23:07 ` Alexei Starovoitov
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2025-07-30 23:02 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Emil Tsalapatis, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman,
Barret Rhoden, Matt Bobrowski, kkd, kernel-team, linux-hardening
On Thu, Jul 03, 2025 at 01:48:14PM -0700, Kumar Kartikeya Dwivedi wrote:
> +static void bpf_prog_report_rqspinlock_violation(const char *str, void *lock, bool irqsave)
> +{
> + struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
> + struct bpf_stream_stage ss;
> + struct bpf_prog *prog;
> +
> + prog = bpf_prog_find_from_stack();
> + if (!prog)
> + return;
> + bpf_stream_stage(ss, prog, BPF_STDERR, ({
> + bpf_stream_printk(ss, "ERROR: %s for bpf_res_spin_lock%s\n", str, irqsave ? "_irqsave" : "");
> + bpf_stream_printk(ss, "Attempted lock = 0x%px\n", lock);
> + bpf_stream_printk(ss, "Total held locks = %d\n", rqh->cnt);
> + for (int i = 0; i < min(RES_NR_HELD, rqh->cnt); i++)
> + bpf_stream_printk(ss, "Held lock[%2d] = 0x%px\n", i, rqh->locks[i]);
> + bpf_stream_dump_stack(ss);
Please don't include %px in stuff going back to userspace in standard
error reporting. That's a kernel address leak:
https://docs.kernel.org/process/deprecated.html#p-format-specifier
I don't see any justification here, please remove the lock address or
use regular %p to get a hashed value.
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr
2025-07-30 23:02 ` [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr Kees Cook
@ 2025-07-30 23:07 ` Alexei Starovoitov
2025-07-30 23:09 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2025-07-30 23:07 UTC (permalink / raw)
To: Kees Cook
Cc: Kumar Kartikeya Dwivedi, bpf, Emil Tsalapatis, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Barret Rhoden, Matt Bobrowski, kkd, Kernel Team,
linux-hardening
On Wed, Jul 30, 2025 at 4:02 PM Kees Cook <kees@kernel.org> wrote:
>
> On Thu, Jul 03, 2025 at 01:48:14PM -0700, Kumar Kartikeya Dwivedi wrote:
> > +static void bpf_prog_report_rqspinlock_violation(const char *str, void *lock, bool irqsave)
> > +{
> > + struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
> > + struct bpf_stream_stage ss;
> > + struct bpf_prog *prog;
> > +
> > + prog = bpf_prog_find_from_stack();
> > + if (!prog)
> > + return;
> > + bpf_stream_stage(ss, prog, BPF_STDERR, ({
> > + bpf_stream_printk(ss, "ERROR: %s for bpf_res_spin_lock%s\n", str, irqsave ? "_irqsave" : "");
> > + bpf_stream_printk(ss, "Attempted lock = 0x%px\n", lock);
> > + bpf_stream_printk(ss, "Total held locks = %d\n", rqh->cnt);
> > + for (int i = 0; i < min(RES_NR_HELD, rqh->cnt); i++)
> > + bpf_stream_printk(ss, "Held lock[%2d] = 0x%px\n", i, rqh->locks[i]);
> > + bpf_stream_dump_stack(ss);
>
> Please don't include %px in stuff going back to userspace in standard
> error reporting. That's a kernel address leak:
> https://docs.kernel.org/process/deprecated.html#p-format-specifier
>
> I don't see any justification here, please remove the lock address or
> use regular %p to get a hashed value.
There is no leak here.
The prog was loaded by root and error is read by root.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr
2025-07-30 23:07 ` Alexei Starovoitov
@ 2025-07-30 23:09 ` Kees Cook
2025-07-30 23:13 ` Alexei Starovoitov
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2025-07-30 23:09 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Emil Tsalapatis, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Barret Rhoden, Matt Bobrowski, kkd, Kernel Team,
linux-hardening
On Wed, Jul 30, 2025 at 04:07:33PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 30, 2025 at 4:02 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Thu, Jul 03, 2025 at 01:48:14PM -0700, Kumar Kartikeya Dwivedi wrote:
> > > +static void bpf_prog_report_rqspinlock_violation(const char *str, void *lock, bool irqsave)
> > > +{
> > > + struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
> > > + struct bpf_stream_stage ss;
> > > + struct bpf_prog *prog;
> > > +
> > > + prog = bpf_prog_find_from_stack();
> > > + if (!prog)
> > > + return;
> > > + bpf_stream_stage(ss, prog, BPF_STDERR, ({
> > > + bpf_stream_printk(ss, "ERROR: %s for bpf_res_spin_lock%s\n", str, irqsave ? "_irqsave" : "");
> > > + bpf_stream_printk(ss, "Attempted lock = 0x%px\n", lock);
> > > + bpf_stream_printk(ss, "Total held locks = %d\n", rqh->cnt);
> > > + for (int i = 0; i < min(RES_NR_HELD, rqh->cnt); i++)
> > > + bpf_stream_printk(ss, "Held lock[%2d] = 0x%px\n", i, rqh->locks[i]);
> > > + bpf_stream_dump_stack(ss);
> >
> > Please don't include %px in stuff going back to userspace in standard
> > error reporting. That's a kernel address leak:
> > https://docs.kernel.org/process/deprecated.html#p-format-specifier
> >
> > I don't see any justification here, please remove the lock address or
> > use regular %p to get a hashed value.
>
> There is no leak here.
> The prog was loaded by root and error is read by root.
uid has nothing to do with it. Leaking addresses needs the right
capability set. Is that always true here?
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr
2025-07-30 23:09 ` Kees Cook
@ 2025-07-30 23:13 ` Alexei Starovoitov
2025-07-30 23:19 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2025-07-30 23:13 UTC (permalink / raw)
To: Kees Cook
Cc: Kumar Kartikeya Dwivedi, bpf, Emil Tsalapatis, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Barret Rhoden, Matt Bobrowski, kkd, Kernel Team,
linux-hardening
On Wed, Jul 30, 2025 at 4:09 PM Kees Cook <kees@kernel.org> wrote:
>
> On Wed, Jul 30, 2025 at 04:07:33PM -0700, Alexei Starovoitov wrote:
> > On Wed, Jul 30, 2025 at 4:02 PM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Thu, Jul 03, 2025 at 01:48:14PM -0700, Kumar Kartikeya Dwivedi wrote:
> > > > +static void bpf_prog_report_rqspinlock_violation(const char *str, void *lock, bool irqsave)
> > > > +{
> > > > + struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
> > > > + struct bpf_stream_stage ss;
> > > > + struct bpf_prog *prog;
> > > > +
> > > > + prog = bpf_prog_find_from_stack();
> > > > + if (!prog)
> > > > + return;
> > > > + bpf_stream_stage(ss, prog, BPF_STDERR, ({
> > > > + bpf_stream_printk(ss, "ERROR: %s for bpf_res_spin_lock%s\n", str, irqsave ? "_irqsave" : "");
> > > > + bpf_stream_printk(ss, "Attempted lock = 0x%px\n", lock);
> > > > + bpf_stream_printk(ss, "Total held locks = %d\n", rqh->cnt);
> > > > + for (int i = 0; i < min(RES_NR_HELD, rqh->cnt); i++)
> > > > + bpf_stream_printk(ss, "Held lock[%2d] = 0x%px\n", i, rqh->locks[i]);
> > > > + bpf_stream_dump_stack(ss);
> > >
> > > Please don't include %px in stuff going back to userspace in standard
> > > error reporting. That's a kernel address leak:
> > > https://docs.kernel.org/process/deprecated.html#p-format-specifier
> > >
> > > I don't see any justification here, please remove the lock address or
> > > use regular %p to get a hashed value.
> >
> > There is no leak here.
> > The prog was loaded by root and error is read by root.
>
> uid has nothing to do with it. Leaking addresses needs the right
> capability set. Is that always true here?
yes. For bpf prog to use this kfunc it needs CAP_BPF and CAP_PERMON.
What's allowed under them is described in include/uapi/linux/capability.h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr
2025-07-30 23:13 ` Alexei Starovoitov
@ 2025-07-30 23:19 ` Kees Cook
0 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2025-07-30 23:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, bpf, Emil Tsalapatis, Alexei Starovoitov,
Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau,
Eduard Zingerman, Barret Rhoden, Matt Bobrowski, kkd, Kernel Team,
linux-hardening
On Wed, Jul 30, 2025 at 04:13:25PM -0700, Alexei Starovoitov wrote:
> On Wed, Jul 30, 2025 at 4:09 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Wed, Jul 30, 2025 at 04:07:33PM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jul 30, 2025 at 4:02 PM Kees Cook <kees@kernel.org> wrote:
> > > >
> > > > On Thu, Jul 03, 2025 at 01:48:14PM -0700, Kumar Kartikeya Dwivedi wrote:
> > > > > +static void bpf_prog_report_rqspinlock_violation(const char *str, void *lock, bool irqsave)
> > > > > +{
> > > > > + struct rqspinlock_held *rqh = this_cpu_ptr(&rqspinlock_held_locks);
> > > > > + struct bpf_stream_stage ss;
> > > > > + struct bpf_prog *prog;
> > > > > +
> > > > > + prog = bpf_prog_find_from_stack();
> > > > > + if (!prog)
> > > > > + return;
> > > > > + bpf_stream_stage(ss, prog, BPF_STDERR, ({
> > > > > + bpf_stream_printk(ss, "ERROR: %s for bpf_res_spin_lock%s\n", str, irqsave ? "_irqsave" : "");
> > > > > + bpf_stream_printk(ss, "Attempted lock = 0x%px\n", lock);
> > > > > + bpf_stream_printk(ss, "Total held locks = %d\n", rqh->cnt);
> > > > > + for (int i = 0; i < min(RES_NR_HELD, rqh->cnt); i++)
> > > > > + bpf_stream_printk(ss, "Held lock[%2d] = 0x%px\n", i, rqh->locks[i]);
> > > > > + bpf_stream_dump_stack(ss);
> > > >
> > > > Please don't include %px in stuff going back to userspace in standard
> > > > error reporting. That's a kernel address leak:
> > > > https://docs.kernel.org/process/deprecated.html#p-format-specifier
> > > >
> > > > I don't see any justification here, please remove the lock address or
> > > > use regular %p to get a hashed value.
> > >
> > > There is no leak here.
> > > The prog was loaded by root and error is read by root.
> >
> > uid has nothing to do with it. Leaking addresses needs the right
> > capability set. Is that always true here?
>
> yes. For bpf prog to use this kfunc it needs CAP_BPF and CAP_PERMON.
> What's allowed under them is described in include/uapi/linux/capability.h
Okay, thanks! That wasn't detailed in the commit log (for folks like me
with less familiarity with BPF), so I just got worried when I saw
"stderr" mentioned. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-07-30 23:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250703204818.925464-1-memxor@gmail.com>
[not found] ` <20250703204818.925464-9-memxor@gmail.com>
2025-07-30 23:02 ` [PATCH bpf-next v5 08/12] bpf: Report rqspinlock deadlocks/timeout to BPF stderr Kees Cook
2025-07-30 23:07 ` Alexei Starovoitov
2025-07-30 23:09 ` Kees Cook
2025-07-30 23:13 ` Alexei Starovoitov
2025-07-30 23:19 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox