diff mbox series

net: remove check before __cgroup_bpf_run_filter_skb

Message ID ngc7klapduckb67tsymb3blu2wlmdsjo4pa4gbaivgxezbwzxp@v7akqu7gbwl4 (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series net: remove check before __cgroup_bpf_run_filter_skb | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization

Commit Message

Oliver Crumrine Feb. 8, 2024, 5:50 p.m. UTC 
Checking if __sk is a full socket in macro 
BPF_CGROUP_RUN_PROG_INET_EGRESS is redundant, as the same check is 
done in function __cgroup_bpf_run_filter_skb, called as part of the 
macro.

Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>
---
 include/linux/bpf-cgroup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Oliver Crumrine Feb. 8, 2024, 9:50 p.m. UTC | #1 
On Thu, Feb 08, 2024 at 04:43:06PM -0800, Stanislav Fomichev wrote:
> The check is here to make sure we only run this hook on non-req sockets.
> Dropping it would mean we'd be running the hook on the listeners
> instead. I don't think we want that.

You are correct that we don't want to run the code on listeners. However
the check for that is in the function this macro calls,
__cgroup_bpf_run_filter_skb (the check is on line 1367 of
kernel/bpf/cgroup.c, for 6.8.0-rc3). The check doesn't need to be done
twice, so it can be removed in this macro.
Stanislav Fomichev Feb. 9, 2024, 12:43 a.m. UTC | #2 
On 02/08, Oliver Crumrine wrote:
> Checking if __sk is a full socket in macro 
> BPF_CGROUP_RUN_PROG_INET_EGRESS is redundant, as the same check is 
> done in function __cgroup_bpf_run_filter_skb, called as part of the 
> macro.

The check is here to make sure we only run this hook on non-req sockets.
Dropping it would mean we'd be running the hook on the listeners
instead. I don't think we want that.
Oliver Crumrine Feb. 9, 2024, 3:36 p.m. UTC | #3 
On Fri, Feb 09, 2024 at 11:00:09AM -0800, Stanislav Fomichev wrote:
> Maybe we should instead remove "(!sk || !sk_fullsock(sk))" check from
> __cgroup_bpf_run_filter_skb? BPF_CGROUP_RUN_PROG_INET_EGRESS makes
> care of all those corner conditions. We just need to add those checks to
> BPF_CGROUP_RUN_PROG_INET_INGRESS.
> 
> Let me also CC Kui-Feng, he was touching this part recently in commit
> 223f5f79f2ce ("bpf, net: Check skb ownership against full socket.").

Completely agree with this -- it would be best from a performance
standpoint. I will send out a v2 of this patch in a few hours.
Stanislav Fomichev Feb. 9, 2024, 7 p.m. UTC | #4 
On 02/08, Oliver Crumrine wrote:
> On Thu, Feb 08, 2024 at 04:43:06PM -0800, Stanislav Fomichev wrote:
> > The check is here to make sure we only run this hook on non-req sockets.
> > Dropping it would mean we'd be running the hook on the listeners
> > instead. I don't think we want that.
> 
> You are correct that we don't want to run the code on listeners. However
> the check for that is in the function this macro calls,
> __cgroup_bpf_run_filter_skb (the check is on line 1367 of
> kernel/bpf/cgroup.c, for 6.8.0-rc3). The check doesn't need to be done
> twice, so it can be removed in this macro. 

Maybe we should instead remove "(!sk || !sk_fullsock(sk))" check from
__cgroup_bpf_run_filter_skb? BPF_CGROUP_RUN_PROG_INET_EGRESS makes
care of all those corner conditions. We just need to add those checks to
BPF_CGROUP_RUN_PROG_INET_INGRESS.

Let me also CC Kui-Feng, he was touching this part recently in commit
223f5f79f2ce ("bpf, net: Check skb ownership against full socket.").
Kui-Feng Lee Feb. 9, 2024, 8:33 p.m. UTC | #5 
On 2/9/24 11:00, Stanislav Fomichev wrote:
> On 02/08, Oliver Crumrine wrote:
>> On Thu, Feb 08, 2024 at 04:43:06PM -0800, Stanislav Fomichev wrote:
>>> The check is here to make sure we only run this hook on non-req sockets.
>>> Dropping it would mean we'd be running the hook on the listeners
>>> instead. I don't think we want that.
>>
>> You are correct that we don't want to run the code on listeners. However
>> the check for that is in the function this macro calls,
>> __cgroup_bpf_run_filter_skb (the check is on line 1367 of
>> kernel/bpf/cgroup.c, for 6.8.0-rc3). The check doesn't need to be done
>> twice, so it can be removed in this macro.
> 
> Maybe we should instead remove "(!sk || !sk_fullsock(sk))" check from
> __cgroup_bpf_run_filter_skb? BPF_CGROUP_RUN_PROG_INET_EGRESS makes
> care of all those corner conditions. We just need to add those checks to
> BPF_CGROUP_RUN_PROG_INET_INGRESS.
> 
> Let me also CC Kui-Feng, he was touching this part recently in commit
> 223f5f79f2ce ("bpf, net: Check skb ownership against full socket.").
> 

Adding those checks in BPF_CGROUP_RUN_PROG_INET_INGRESS makes sense
to me if the point here is saving CPU cycles during runtime.
diff mbox series

Patch

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index a789266feac3..95b4a4715d60 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -208,7 +208,7 @@  static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
 	int __ret = 0;							       \
 	if (cgroup_bpf_enabled(CGROUP_INET_EGRESS) && sk) {		       \
 		typeof(sk) __sk = sk_to_full_sk(sk);			       \
-		if (sk_fullsock(__sk) && __sk == skb_to_full_sk(skb) &&	       \
+		if (__sk == skb_to_full_sk(skb) &&			       \
 		    cgroup_bpf_sock_enabled(__sk, CGROUP_INET_EGRESS))	       \
 			__ret = __cgroup_bpf_run_filter_skb(__sk, skb,	       \
 						      CGROUP_INET_EGRESS); \