[PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[xu.xin16]

From: xu xin <xu.xin16@zte.com.cn>

Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
code from bpf_trampoline_unlink_prog(). This aids debugging of race
conditions during link teardown, while keeping the warning rate limited
to avoid log flooding.

Signed-off-by: xu xin <xu.xin16@zte.com.cn>
---
 kernel/bpf/syscall.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6db306d23b47..2348dc33abf4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3626,10 +3626,13 @@ static void bpf_tracing_link_release(struct bpf_link *link)
 {
 	struct bpf_tracing_link *tr_link =
 		container_of(link, struct bpf_tracing_link, link.link);
+	int err;

-	WARN_ON_ONCE(bpf_trampoline_unlink_prog(&tr_link->link.node,
-						tr_link->trampoline,
-						tr_link->tgt_prog));
+	err = bpf_trampoline_unlink_prog(&tr_link->link.node,
+					tr_link->trampoline,
+					tr_link->tgt_prog);
+	if (err)
+		WARN_ONCE(err, "bpf_trampoline_unlink_prog returns error: %d\n", err);

 	bpf_trampoline_put(tr_link->trampoline);

-- 
2.25.1

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[Leon Hwang]

In subject, 'bpf:' is enough, 'syscall:' can be dropped.

On 16/7/26 11:34, xu.xin16@zte.com.cn wrote:
> From: xu xin <xu.xin16@zte.com.cn>
> 
> Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
> code from bpf_trampoline_unlink_prog(). This aids debugging of race
> conditions during link teardown, while keeping the warning rate limited
> to avoid log flooding.


Have you tried to trace bpf_trampoline_unlink_prog() with fexit?

bpftrace -lv 'fexit:bpf_trampoline_unlink_prog'
kretfunc:vmlinux:bpf_trampoline_unlink_prog
    struct bpf_tramp_node * node
    struct bpf_trampoline * tr
    struct bpf_prog * tgt_prog
    int retval

Since bpf_trampoline_unlink_prog() can be traced with fexit, it seems
unnecessary for this change?

Thanks,
Leon

> [...]

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[xu.xin16]

> In subject, 'bpf:' is enough, 'syscall:' can be dropped.

Ok, thanks.

> 
> On 16/7/26 11:34, xu.xin16@zte.com.cn wrote:
> > From: xu xin <xu.xin16@zte.com.cn>
> > 
> > Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
> > code from bpf_trampoline_unlink_prog(). This aids debugging of race
> > conditions during link teardown, while keeping the warning rate limited
> > to avoid log flooding.
> 
> 
> Have you tried to trace bpf_trampoline_unlink_prog() with fexit?
> 
> bpftrace -lv 'fexit:bpf_trampoline_unlink_prog'
> kretfunc:vmlinux:bpf_trampoline_unlink_prog
>     struct bpf_tramp_node * node
>     struct bpf_trampoline * tr
>     struct bpf_prog * tgt_prog
>     int retval
> 
> Since bpf_trampoline_unlink_prog() can be traced with fexit, it seems
> unnecessary for this change?

Well, the crash may happen when the kernel is booting, or too hard to reproduce,
in which case, bpftrace does not work.

So this change will be more helpful, I believe.

Thanks,
xu xin

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[Leon Hwang]

On 16/7/26 13:55, xu.xin16@zte.com.cn wrote:
>> In subject, 'bpf:' is enough, 'syscall:' can be dropped.
> 
> Ok, thanks.
> 
>>
>> On 16/7/26 11:34, xu.xin16@zte.com.cn wrote:
>>> From: xu xin <xu.xin16@zte.com.cn>
>>>
>>> Replace silent WARN_ON_ONCE with WARN_ONCE that prints the actual error
>>> code from bpf_trampoline_unlink_prog(). This aids debugging of race
>>> conditions during link teardown, while keeping the warning rate limited
>>> to avoid log flooding.
>>
>>
>> Have you tried to trace bpf_trampoline_unlink_prog() with fexit?
>>
>> bpftrace -lv 'fexit:bpf_trampoline_unlink_prog'
>> kretfunc:vmlinux:bpf_trampoline_unlink_prog
>>     struct bpf_tramp_node * node
>>     struct bpf_trampoline * tr
>>     struct bpf_prog * tgt_prog
>>     int retval
>>
>> Since bpf_trampoline_unlink_prog() can be traced with fexit, it seems
>> unnecessary for this change?
> 
> Well, the crash may happen when the kernel is booting, or too hard to reproduce,
> in which case, bpftrace does not work.


Do you enable kernel.panic_on_warn sysctl?

If it's not enabled, the kernel won't crash, I think.

Thanks,
Leon

> 
> So this change will be more helpful, I believe.
> 
> Thanks,
> xu xin
> 

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[xu.xin16]

> > Well, the crash may happen when the kernel is booting, or too hard to reproduce,
> > in which case, bpftrace does not work.
> 
> 
> Do you enable kernel.panic_on_warn sysctl?
> 
> If it's not enabled, the kernel won't crash, I think.

I'd like to clarify that the panic in our case is not triggered by the WARN itself.
Instead, the real crash is a use‑after‑free (UAF) that happens when one CPU is
executing a trampoline while another CPU is concurrently freeing it. This results
in an illegal instruction (or page fault) at the PC, which is fatal and will panic
the kernel regardless of panic_on_warn – because it's an Oops with Fatal exception
in interrupt.

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[Leon Hwang]

On 16/7/26 14:26, xu.xin16@zte.com.cn wrote:
>>> Well, the crash may happen when the kernel is booting, or too hard to reproduce,
>>> in which case, bpftrace does not work.
>>
>>
>> Do you enable kernel.panic_on_warn sysctl?
>>
>> If it's not enabled, the kernel won't crash, I think.
> 
> I'd like to clarify that the panic in our case is not triggered by the WARN itself.
> Instead, the real crash is a use‑after‑free (UAF) that happens when one CPU is
> executing a trampoline while another CPU is concurrently freeing it. This results
> in an illegal instruction (or page fault) at the PC, which is fatal and will panic
> the kernel regardless of panic_on_warn – because it's an Oops with Fatal exception
> in interrupt.


Why not fix the root cause instead?

If you want some help from upstream to fix the root cause, pls provide
the crash dmesg and the reproducing demo.

Thanks,
Leon

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[xu.xin16]

> >>> Well, the crash may happen when the kernel is booting, or too hard to reproduce,
> >>> in which case, bpftrace does not work.
> >>
> >>
> >> Do you enable kernel.panic_on_warn sysctl?
> >>
> >> If it's not enabled, the kernel won't crash, I think.
> > 
> > I'd like to clarify that the panic in our case is not triggered by the WARN itself.
> > Instead, the real crash is a use‑after‑free (UAF) that happens when one CPU is
> > executing a trampoline while another CPU is concurrently freeing it. This results
> > in an illegal instruction (or page fault) at the PC, which is fatal and will panic
> > the kernel regardless of panic_on_warn – because it's an Oops with Fatal exception
> > in interrupt.
> 
> 
> Why not fix the root cause instead?
> 
> If you want some help from upstream to fix the root cause, pls provide
> the crash dmesg and the reproducing demo.

It's already fixed due to some OOT drivers.

I think if we have this patch, it can speed up trouble-shooting.

Re: [PATCH bpf-next] bpf:syscall: Log error code on trampoline unlink failure

[Leon Hwang]

On 16/7/26 15:03, xu.xin16@zte.com.cn wrote:
>>>>> Well, the crash may happen when the kernel is booting, or too hard to reproduce,
>>>>> in which case, bpftrace does not work.
>>>>
>>>>
>>>> Do you enable kernel.panic_on_warn sysctl?
>>>>
>>>> If it's not enabled, the kernel won't crash, I think.
>>>
>>> I'd like to clarify that the panic in our case is not triggered by the WARN itself.
>>> Instead, the real crash is a use‑after‑free (UAF) that happens when one CPU is
>>> executing a trampoline while another CPU is concurrently freeing it. This results
>>> in an illegal instruction (or page fault) at the PC, which is fatal and will panic
>>> the kernel regardless of panic_on_warn – because it's an Oops with Fatal exception
>>> in interrupt.
>>
>>
>> Why not fix the root cause instead?
>>
>> If you want some help from upstream to fix the root cause, pls provide
>> the crash dmesg and the reproducing demo.
> 
> It's already fixed due to some OOT drivers.
> 
> I think if we have this patch, it can speed up trouble-shooting.

OK. Let's wait for Jiri's opinion.

Thanks,
Leon