[syzbot] [bpf?] [net?] WARNING in bpf_lwt_seg6_adjust_srh

[syzbot] [bpf?] [net?] WARNING in bpf_lwt_seg6_adjust_srh

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    bf2468f9afba Merge branch 'locking-introduce-nested-bh-loc..
git tree:       net-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13cb0aea980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=e78fc116033e0ab7
dashboard link: https://syzkaller.appspot.com/bug?extid=608a2acde8c5a101d07d
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12eaa3ea980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=15eff72e980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/f3b564f7e07c/disk-bf2468f9.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/cd47135279ed/vmlinux-bf2468f9.xz
kernel image: https://storage.googleapis.com/syzbot-assets/ccb81d174cf6/bzImage-bf2468f9.xz

The issue was bisected to:

commit d1542d4ae4dfdc47c9b3205ebe849ed23af213dd
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date:   Thu Jun 20 13:22:02 2024 +0000

    seg6: Use nested-BH locking for seg6_bpf_srh_states.

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=10311e2a980000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=12311e2a980000
console output: https://syzkaller.appspot.com/x/log.txt?x=14311e2a980000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+608a2acde8c5a101d07d@syzkaller.appspotmail.com
Fixes: d1542d4ae4df ("seg6: Use nested-BH locking for seg6_bpf_srh_states.")

------------[ cut here ]------------
WARNING: CPU: 0 PID: 5091 at net/core/filter.c:6579 ____bpf_lwt_seg6_adjust_srh net/core/filter.c:6579 [inline]
WARNING: CPU: 0 PID: 5091 at net/core/filter.c:6579 bpf_lwt_seg6_adjust_srh+0x877/0xb30 net/core/filter.c:6568
Modules linked in:
CPU: 0 PID: 5091 Comm: syz-executor570 Not tainted 6.10.0-rc4-syzkaller-00891-gbf2468f9afba #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/07/2024
RIP: 0010:____bpf_lwt_seg6_adjust_srh net/core/filter.c:6579 [inline]
RIP: 0010:bpf_lwt_seg6_adjust_srh+0x877/0xb30 net/core/filter.c:6568
Code: bf 80 33 f8 eb 05 e8 b8 80 33 f8 48 c7 c0 f2 ff ff ff e9 d1 fc ff ff e8 a7 80 33 f8 48 63 c3 e9 c4 fc ff ff e8 9a 80 33 f8 90 <0f> 0b 90 4d 85 f6 0f 85 0e f9 ff ff e9 46 fa ff ff e8 83 80 33 f8
RSP: 0018:ffffc900034a77a0 EFLAGS: 00010293
RAX: ffffffff8962a486 RBX: 0000000000000000 RCX: ffff888017fdda00
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffc900034a78d8 R08: ffffffff89629d8b R09: 1ffffffff1f5b52d
R10: dffffc0000000000 R11: ffffffffa00007d0 R12: 0000000000000000
R13: ffff8880b943d060 R14: 0000000000000000 R15: dffffc0000000000
FS:  000055555b006380(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000564def260000 CR3: 00000000775ec000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 bpf_prog_2088341bddeddc1d+0x40/0x42
 bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
 __bpf_prog_run include/linux/filter.h:691 [inline]
 bpf_prog_run include/linux/filter.h:698 [inline]
 bpf_test_run+0x4f0/0xa90 net/bpf/test_run.c:432
 bpf_prog_test_run_skb+0xafa/0x13b0 net/bpf/test_run.c:1081
 bpf_prog_test_run+0x33a/0x3b0 kernel/bpf/syscall.c:4313
 __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5728
 __do_sys_bpf kernel/bpf/syscall.c:5817 [inline]
 __se_sys_bpf kernel/bpf/syscall.c:5815 [inline]
 __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5815
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f541c355529
Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffca122c488 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
RAX: ffffffffffffffda RBX: 00007ffca122c658 RCX: 00007f541c355529
RDX: 0000000000000050 RSI: 00000000200002c0 RDI: 000000000000000a
RBP: 00007f541c3c8610 R08: 0000000000000000 R09: 00007ffca122c658
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffca122c648 R14: 0000000000000001 R15: 0000000000000001
 </TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [bpf?] [net?] WARNING in bpf_lwt_seg6_adjust_srh

[Sebastian Andrzej Siewior]

On 2024-06-25 09:51:25 [-0700], syzbot wrote:
> Hello,
Hi,

…
> commit d1542d4ae4dfdc47c9b3205ebe849ed23af213dd
> Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date:   Thu Jun 20 13:22:02 2024 +0000
> 
>     seg6: Use nested-BH locking for seg6_bpf_srh_states.
…
> WARNING: CPU: 0 PID: 5091 at net/core/filter.c:6579 ____bpf_lwt_seg6_adjust_srh net/core/filter.c:6579 [inline]
> WARNING: CPU: 0 PID: 5091 at net/core/filter.c:6579 bpf_lwt_seg6_adjust_srh+0x877/0xb30 net/core/filter.c:6568
…
> Call Trace:
>  <TASK>
>  bpf_prog_2088341bddeddc1d+0x40/0x42
>  bpf_dispatcher_nop_func include/linux/bpf.h:1243 [inline]
>  __bpf_prog_run include/linux/filter.h:691 [inline]
>  bpf_prog_run include/linux/filter.h:698 [inline]
>  bpf_test_run+0x4f0/0xa90 net/bpf/test_run.c:432
>  bpf_prog_test_run_skb+0xafa/0x13b0 net/bpf/test_run.c:1081
>  bpf_prog_test_run+0x33a/0x3b0 kernel/bpf/syscall.c:4313
>  __sys_bpf+0x48d/0x810 kernel/bpf/syscall.c:5728
>  __do_sys_bpf kernel/bpf/syscall.c:5817 [inline]
>  __se_sys_bpf kernel/bpf/syscall.c:5815 [inline]
>  __x64_sys_bpf+0x7c/0x90 kernel/bpf/syscall.c:5815
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f

I assumed this can only originate from input_action_end_bpf() but
clearly this not a hard requirement based on the report.
So this a valid invocation and it should not have been killer earlier in
the stack?

Sebastian

[PATCH bpf-next] seg6: Ensure that seg6_bpf_srh_states can only be accessed from input_action_end_bpf()

[Sebastian Andrzej Siewior]

Initially I assumed that the per-CPU variable is `seg6_bpf_srh_states'
is first initialized in input_action_end_bpf() and then accessed during
the bpf_prog_run_save_cb() invocation by the eBPF via the BPF callbacks.
syzbot demonstrated that is possible to invoke the BPF callbacks (and
access `seg6_bpf_srh_states') without entering input_action_end_bpf()
first.

The valid path via input_action_end_bpf() is invoked within NAPI
context which means it has bpf_net_context set. This can be used to
identify the "valid" calling path.

Set in input_action_end_bpf() the BPF_RI_F_SEG6_STATE bit to signal the
valid calling path and clear it at the end. Check for the context and
the bit in bpf_lwt_seg6.*() and abort if missing.

Reported-by: syzbot+608a2acde8c5a101d07d@syzkaller.appspotmail.com
Fixes: d1542d4ae4dfd ("seg6: Use nested-BH locking for seg6_bpf_srh_states.")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/filter.h | 24 ++++++++++++++++++++++++
 net/core/filter.c      |  6 ++++++
 net/ipv6/seg6_local.c  |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 0bbd2585e6def..cadddb25ff4db 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -739,6 +739,7 @@ struct bpf_nh_params {
 #define BPF_RI_F_CPU_MAP_INIT	BIT(2)
 #define BPF_RI_F_DEV_MAP_INIT	BIT(3)
 #define BPF_RI_F_XSK_MAP_INIT	BIT(4)
+#define BPF_RI_F_SEG6_STATE	BIT(5)
 
 struct bpf_redirect_info {
 	u64 tgt_index;
@@ -856,6 +857,29 @@ static inline void bpf_net_ctx_get_all_used_flush_lists(struct list_head **lh_ma
 		*lh_xsk = lh;
 }
 
+static inline bool bpf_net_ctx_seg6_state_avail(void)
+{
+	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
+
+	if (!bpf_net_ctx)
+		return false;
+	return bpf_net_ctx->ri.kern_flags & BPF_RI_F_SEG6_STATE;
+}
+
+static inline void bpf_net_ctx_seg6_state_set(void)
+{
+	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
+
+	bpf_net_ctx->ri.kern_flags |= BPF_RI_F_SEG6_STATE;
+}
+
+static inline void bpf_net_ctx_seg6_state_clr(void)
+{
+	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
+
+	bpf_net_ctx->ri.kern_flags &= ~BPF_RI_F_SEG6_STATE;
+}
+
 /* Compute the linear packet data range [data, data_end) which
  * will be accessed by various program types (cls_bpf, act_bpf,
  * lwt, ...). Subsystems allowing direct data access must (!)
diff --git a/net/core/filter.c b/net/core/filter.c
index 403d23faf22e1..ea5bc4a4a6a23 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6459,6 +6459,8 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
 	void *srh_tlvs, *srh_end, *ptr;
 	int srhoff = 0;
 
+	if (!bpf_net_ctx_seg6_state_avail())
+		return -EINVAL;
 	lockdep_assert_held(&srh_state->bh_lock);
 	if (srh == NULL)
 		return -EINVAL;
@@ -6516,6 +6518,8 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 	int hdroff = 0;
 	int err;
 
+	if (!bpf_net_ctx_seg6_state_avail())
+		return -EINVAL;
 	lockdep_assert_held(&srh_state->bh_lock);
 	switch (action) {
 	case SEG6_LOCAL_ACTION_END_X:
@@ -6593,6 +6597,8 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
 	int srhoff = 0;
 	int ret;
 
+	if (!bpf_net_ctx_seg6_state_avail())
+		return -EINVAL;
 	lockdep_assert_held(&srh_state->bh_lock);
 	if (unlikely(srh == NULL))
 		return -EINVAL;
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index c74705ead9849..3e3a48b7266b5 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -1429,6 +1429,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
 	 * bpf_prog_run_save_cb().
 	 */
 	local_lock_nested_bh(&seg6_bpf_srh_states.bh_lock);
+	bpf_net_ctx_seg6_state_set();
 	srh_state = this_cpu_ptr(&seg6_bpf_srh_states);
 	srh_state->srh = srh;
 	srh_state->hdrlen = srh->hdrlen << 3;
@@ -1452,6 +1453,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
 
 	if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
 		goto drop;
+	bpf_net_ctx_seg6_state_clr();
 	local_unlock_nested_bh(&seg6_bpf_srh_states.bh_lock);
 
 	if (ret != BPF_REDIRECT)
@@ -1460,6 +1462,7 @@ static int input_action_end_bpf(struct sk_buff *skb,
 	return dst_input(skb);
 
 drop:
+	bpf_net_ctx_seg6_state_clr();
 	local_unlock_nested_bh(&seg6_bpf_srh_states.bh_lock);
 	kfree_skb(skb);
 	return -EINVAL;
-- 
2.45.2

Re: [PATCH bpf-next] seg6: Ensure that seg6_bpf_srh_states can only be accessed from input_action_end_bpf()

[Martin KaFai Lau]

On 7/5/24 3:41 AM, Sebastian Andrzej Siewior wrote:
> Initially I assumed that the per-CPU variable is `seg6_bpf_srh_states'
> is first initialized in input_action_end_bpf() and then accessed during
> the bpf_prog_run_save_cb() invocation by the eBPF via the BPF callbacks.
> syzbot demonstrated that is possible to invoke the BPF callbacks (and
> access `seg6_bpf_srh_states') without entering input_action_end_bpf()
> first.
> 
> The valid path via input_action_end_bpf() is invoked within NAPI
> context which means it has bpf_net_context set. This can be used to
> identify the "valid" calling path.
> 
> Set in input_action_end_bpf() the BPF_RI_F_SEG6_STATE bit to signal the
> valid calling path and clear it at the end. Check for the context and
> the bit in bpf_lwt_seg6.*() and abort if missing.
> 
> Reported-by: syzbot+608a2acde8c5a101d07d@syzkaller.appspotmail.com
> Fixes: d1542d4ae4dfd ("seg6: Use nested-BH locking for seg6_bpf_srh_states.")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   include/linux/filter.h | 24 ++++++++++++++++++++++++
>   net/core/filter.c      |  6 ++++++
>   net/ipv6/seg6_local.c  |  3 +++
>   3 files changed, 33 insertions(+)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 0bbd2585e6def..cadddb25ff4db 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -739,6 +739,7 @@ struct bpf_nh_params {
>   #define BPF_RI_F_CPU_MAP_INIT	BIT(2)
>   #define BPF_RI_F_DEV_MAP_INIT	BIT(3)
>   #define BPF_RI_F_XSK_MAP_INIT	BIT(4)
> +#define BPF_RI_F_SEG6_STATE	BIT(5)
>   
>   struct bpf_redirect_info {
>   	u64 tgt_index;
> @@ -856,6 +857,29 @@ static inline void bpf_net_ctx_get_all_used_flush_lists(struct list_head **lh_ma
>   		*lh_xsk = lh;
>   }
>   
> +static inline bool bpf_net_ctx_seg6_state_avail(void)
> +{
> +	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
> +
> +	if (!bpf_net_ctx)
> +		return false;
> +	return bpf_net_ctx->ri.kern_flags & BPF_RI_F_SEG6_STATE;
> +}
> +
> +static inline void bpf_net_ctx_seg6_state_set(void)
> +{
> +	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
> +
> +	bpf_net_ctx->ri.kern_flags |= BPF_RI_F_SEG6_STATE;
> +}
> +
> +static inline void bpf_net_ctx_seg6_state_clr(void)
> +{
> +	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
> +
> +	bpf_net_ctx->ri.kern_flags &= ~BPF_RI_F_SEG6_STATE;
> +}
> +
>   /* Compute the linear packet data range [data, data_end) which
>    * will be accessed by various program types (cls_bpf, act_bpf,
>    * lwt, ...). Subsystems allowing direct data access must (!)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 403d23faf22e1..ea5bc4a4a6a23 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6459,6 +6459,8 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>   	void *srh_tlvs, *srh_end, *ptr;
>   	int srhoff = 0;
>   
> +	if (!bpf_net_ctx_seg6_state_avail())
> +		return -EINVAL;

The syzbot stack shows that the seg6local bpf_prog can be run by test_run like: 
bpf_prog_test_run_skb() => bpf_test_run(). "return -EINVAL;" will reject and 
break the existing bpf prog doing test with test_run.

bpf_test_run() has already done the local_bh_disable() and bpf_net_ctx_set(). 
How about doing the local_[un]lock_nested_bh(&seg6_bpf_srh_states.bh_lock) in 
bpf_test_run() when the prog->type == BPF_PROG_TYPE_LWT_SEG6LOCAL?

pw-bot: cr

Re: [PATCH bpf-next] seg6: Ensure that seg6_bpf_srh_states can only be accessed from input_action_end_bpf()

[Sebastian Andrzej Siewior]

On 2024-07-08 17:03:58 [-0700], Martin KaFai Lau wrote:
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 403d23faf22e1..ea5bc4a4a6a23 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6459,6 +6459,8 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
> >   	void *srh_tlvs, *srh_end, *ptr;
> >   	int srhoff = 0;
> > +	if (!bpf_net_ctx_seg6_state_avail())
> > +		return -EINVAL;
> 
> The syzbot stack shows that the seg6local bpf_prog can be run by test_run
> like: bpf_prog_test_run_skb() => bpf_test_run(). "return -EINVAL;" will
> reject and break the existing bpf prog doing test with test_run.

But wouldn't this be the case anyway because seg6_bpf_srh_states::srh
isn't assigned?

> bpf_test_run() has already done the local_bh_disable() and
> bpf_net_ctx_set(). How about doing the
> local_[un]lock_nested_bh(&seg6_bpf_srh_states.bh_lock) in bpf_test_run()
> when the prog->type == BPF_PROG_TYPE_LWT_SEG6LOCAL?

Okay. Sure. And I assume it is limited that only those two call paths
can invoke this type of BPF program.

Sebastian

Re: [PATCH bpf-next] seg6: Ensure that seg6_bpf_srh_states can only be accessed from input_action_end_bpf()

[Martin KaFai Lau]

On 7/8/24 10:18 PM, Sebastian Andrzej Siewior wrote:
> On 2024-07-08 17:03:58 [-0700], Martin KaFai Lau wrote:
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 403d23faf22e1..ea5bc4a4a6a23 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -6459,6 +6459,8 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>>>    	void *srh_tlvs, *srh_end, *ptr;
>>>    	int srhoff = 0;
>>> +	if (!bpf_net_ctx_seg6_state_avail())
>>> +		return -EINVAL;
>>
>> The syzbot stack shows that the seg6local bpf_prog can be run by test_run
>> like: bpf_prog_test_run_skb() => bpf_test_run(). "return -EINVAL;" will
>> reject and break the existing bpf prog doing test with test_run.
> 
> But wouldn't this be the case anyway because seg6_bpf_srh_states::srh
> isn't assigned?

Good point.

I don't think the test_run for BPF_PROG_TYPE_LWT_SEG6LOCAL ever works. It seems 
no test_run selftest was ever added to exercise this code path since the 
lwt_seg6local_prog_ops was added in commit 004d4b274e2a ("ipv6: sr: Add 
seg6local action End.BPF").

I think the right thing to do here is to remove the test_run code path for 
BPF_PROG_TYPE_LWT_SEG6LOCAL instead of further patching it. A separate patch for 
proper test_run support is needed (cc: Mathieu Xhonneux).

To remove test_run for BPF_PROG_TYPE_LWT_SEG6LOCAL, this should do:

diff --git i/net/core/filter.c w/net/core/filter.c
index 403d23faf22e..db5e59f62b35 100644
--- i/net/core/filter.c
+++ w/net/core/filter.c
@@ -11053,7 +11053,6 @@ const struct bpf_verifier_ops lwt_seg6local_verifier_ops = {
  };

  const struct bpf_prog_ops lwt_seg6local_prog_ops = {
-	.test_run		= bpf_prog_test_run_skb,
  };

  const struct bpf_verifier_ops cg_sock_verifier_ops = {

Then the bpf_lwt_seg6_* helpers should stay in the input_action_end_bpf() code 
path only.