Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

seg6: Use nested-BH locking for seg6_bpf_srh_states.

The access to seg6_bpf_srh_states is protected by disabling preemption.
Based on the code, the entry point is input_action_end_bpf() and
every other function (the bpf helper functions bpf_lwt_seg6_*()), that
is accessing seg6_bpf_srh_states, should be called from within
input_action_end_bpf().

input_action_end_bpf() accesses seg6_bpf_srh_states first at the top of
the function and then disables preemption. This looks wrong because if
preemption needs to be disabled as part of the locking mechanism then
the variable shouldn't be accessed beforehand.

Looking at how it is used via test_lwt_seg6local.sh then
input_action_end_bpf() is always invoked from softirq context. If this
is always the case then the preempt_disable() statement is superfluous.
If this is not always invoked from softirq then disabling only
preemption is not sufficient.

Replace the preempt_disable() statement with nested-BH locking. This is
not an equivalent replacement as it assumes that the invocation of
input_action_end_bpf() always occurs in softirq context and thus the
preempt_disable() is superfluous.
Add a local_lock_t the data structure and use local_lock_nested_bh() for
locking. Add lockdep_assert_held() to ensure the lock is held while the
per-CPU variable is referenced in the helper functions.

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: David Ahern <dsahern@kernel.org>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://patch.msgid.link/20240620132727.660738-13-bigeasy@linutronix.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

authored by

Sebastian Andrzej Siewior and committed by
Jakub Kicinski
d1542d4a 3414adbd

+18 -8
+1
include/net/seg6_local.h
··· 19 19 extern bool seg6_bpf_has_valid_srh(struct sk_buff *skb); 20 20 21 21 struct seg6_bpf_srh_state { 22 + local_lock_t bh_lock; 22 23 struct ipv6_sr_hdr *srh; 23 24 u16 hdrlen; 24 25 bool valid;
+3
net/core/filter.c
··· 6455 6455 void *srh_tlvs, *srh_end, *ptr; 6456 6456 int srhoff = 0; 6457 6457 6458 + lockdep_assert_held(&srh_state->bh_lock); 6458 6459 if (srh == NULL) 6459 6460 return -EINVAL; 6460 6461 ··· 6512 6511 int hdroff = 0; 6513 6512 int err; 6514 6513 6514 + lockdep_assert_held(&srh_state->bh_lock); 6515 6515 switch (action) { 6516 6516 case SEG6_LOCAL_ACTION_END_X: 6517 6517 if (!seg6_bpf_has_valid_srh(skb)) ··· 6589 6587 int srhoff = 0; 6590 6588 int ret; 6591 6589 6590 + lockdep_assert_held(&srh_state->bh_lock); 6592 6591 if (unlikely(srh == NULL)) 6593 6592 return -EINVAL; 6594 6593
+14 -8
net/ipv6/seg6_local.c
··· 1380 1380 return err; 1381 1381 } 1382 1382 1383 - DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states); 1383 + DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states) = { 1384 + .bh_lock = INIT_LOCAL_LOCK(bh_lock), 1385 + }; 1384 1386 1385 1387 bool seg6_bpf_has_valid_srh(struct sk_buff *skb) 1386 1388 { ··· 1390 1388 this_cpu_ptr(&seg6_bpf_srh_states); 1391 1389 struct ipv6_sr_hdr *srh = srh_state->srh; 1392 1390 1391 + lockdep_assert_held(&srh_state->bh_lock); 1393 1392 if (unlikely(srh == NULL)) 1394 1393 return false; 1395 1394 ··· 1411 1408 static int input_action_end_bpf(struct sk_buff *skb, 1412 1409 struct seg6_local_lwt *slwt) 1413 1410 { 1414 - struct seg6_bpf_srh_state *srh_state = 1415 - this_cpu_ptr(&seg6_bpf_srh_states); 1411 + struct seg6_bpf_srh_state *srh_state; 1416 1412 struct ipv6_sr_hdr *srh; 1417 1413 int ret; 1418 1414 ··· 1422 1420 } 1423 1421 advance_nextseg(srh, &ipv6_hdr(skb)->daddr); 1424 1422 1425 - /* preempt_disable is needed to protect the per-CPU buffer srh_state, 1426 - * which is also accessed by the bpf_lwt_seg6_* helpers 1423 + /* The access to the per-CPU buffer srh_state is protected by running 1424 + * always in softirq context (with disabled BH). On PREEMPT_RT the 1425 + * required locking is provided by the following local_lock_nested_bh() 1426 + * statement. It is also accessed by the bpf_lwt_seg6_* helpers via 1427 + * bpf_prog_run_save_cb(). 1427 1428 */ 1428 - preempt_disable(); 1429 + local_lock_nested_bh(&seg6_bpf_srh_states.bh_lock); 1430 + srh_state = this_cpu_ptr(&seg6_bpf_srh_states); 1429 1431 srh_state->srh = srh; 1430 1432 srh_state->hdrlen = srh->hdrlen << 3; 1431 1433 srh_state->valid = true; ··· 1452 1446 1453 1447 if (srh_state->srh && !seg6_bpf_has_valid_srh(skb)) 1454 1448 goto drop; 1449 + local_unlock_nested_bh(&seg6_bpf_srh_states.bh_lock); 1455 1450 1456 - preempt_enable(); 1457 1451 if (ret != BPF_REDIRECT) 1458 1452 seg6_lookup_nexthop(skb, NULL, 0); 1459 1453 1460 1454 return dst_input(skb); 1461 1455 1462 1456 drop: 1463 - preempt_enable(); 1457 + local_unlock_nested_bh(&seg6_bpf_srh_states.bh_lock); 1464 1458 kfree_skb(skb); 1465 1459 return -EINVAL; 1466 1460 }