diff mbox series

[v4,bpf-next] net: remove check in __cgroup_bpf_run_filter_skb

Message ID 7lv62yiyvmj5a7eozv2iznglpkydkdfancgmbhiptrgvgan5sy@3fl3onchgdz3 (mailing list archive)
State Accepted
Commit 32e18e7688c6847b0c9db073aafb00639ecf576c
Delegated to: BPF
Headers show
Series [v4,bpf-next] net: remove check in __cgroup_bpf_run_filter_skb | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2031 this patch: 2031
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 1098 this patch: 1098
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2074 this patch: 2074
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
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-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-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-42 success Logs for x86_64-llvm-18 / veristat
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-24 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-35 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-33 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-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-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
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-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / veristat
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-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-31 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 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-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 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-18 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-17 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 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-19 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-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 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-32 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-34 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-36 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Oliver Crumrine Feb. 9, 2024, 7:41 p.m. UTC
Originally, this patch removed a redundant check in
BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
and add the checks to the other macro that calls that function,
BPF_CGROUP_RUN_PROG_INET_INGRESS.

To sum it up, checking that the socket exists and that it is a full
socket is now part of both macros BPF_CGROUP_RUN_PROG_INET_EGRESS and
BPF_CGROUP_RUN_PROG_INET_INGRESS, and it is no longer part of the
function they call, __cgroup_bpf_run_filter_skb.

Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>

v3->v4: Fixed weird merge conflict.
v2->v3: Sent to bpf-next instead of generic patch
v1->v2: Addressed feedback about where check should be removed.
---
 include/linux/bpf-cgroup.h | 5 +++--
 kernel/bpf/cgroup.c        | 3 ---
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Stanislav Fomichev Feb. 12, 2024, 4:49 p.m. UTC | #1
On 02/09, Oliver Crumrine wrote:
> Originally, this patch removed a redundant check in
> BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
> the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
> reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
> and add the checks to the other macro that calls that function,
> BPF_CGROUP_RUN_PROG_INET_INGRESS.
> 
> To sum it up, checking that the socket exists and that it is a full
> socket is now part of both macros BPF_CGROUP_RUN_PROG_INET_EGRESS and
> BPF_CGROUP_RUN_PROG_INET_INGRESS, and it is no longer part of the
> function they call, __cgroup_bpf_run_filter_skb.
> 
> Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>

Acked-by: Stanislav Fomichev <sdf@google.com>
Oliver Crumrine Feb. 13, 2024, 6:37 p.m. UTC | #2
On Mon, Feb 12, 2024 at 08:49:14AM -0800, Stanislav Fomichev wrote:
> On 02/09, Oliver Crumrine wrote:
> > Originally, this patch removed a redundant check in
> > BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
> > the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
> > reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
> > and add the checks to the other macro that calls that function,
> > BPF_CGROUP_RUN_PROG_INET_INGRESS.
> > 
> > To sum it up, checking that the socket exists and that it is a full
> > socket is now part of both macros BPF_CGROUP_RUN_PROG_INET_EGRESS and
> > BPF_CGROUP_RUN_PROG_INET_INGRESS, and it is no longer part of the
> > function they call, __cgroup_bpf_run_filter_skb.
> > 
> > Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>
> 
> Acked-by: Stanislav Fomichev <sdf@google.com>

Quick question: My subject had "net:" in it. Should it have had "bpf:" in
the subject instead?

If yes, would this warrant another version of this patch or resending it
with a different subject?

It felt right to put net: there as it felt like I was working with 
networking code that was simply calling bpf code but I'm not exactly
sure of that anymore.

This is my first kernel patch that has actually gone anywhere and 
I'm just looking for some feedback as I couldn't find much good 
documentation on kernel.org that describes how I should be doing 
this.

Thanks, 
Oliver
Martin KaFai Lau Feb. 13, 2024, 11:47 p.m. UTC | #3
On 2/13/24 10:37 AM, Oliver Crumrine wrote:
> On Mon, Feb 12, 2024 at 08:49:14AM -0800, Stanislav Fomichev wrote:
>> On 02/09, Oliver Crumrine wrote:
>>> Originally, this patch removed a redundant check in
>>> BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
>>> the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
>>> reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
>>> and add the checks to the other macro that calls that function,
>>> BPF_CGROUP_RUN_PROG_INET_INGRESS.
>>>
>>> To sum it up, checking that the socket exists and that it is a full
>>> socket is now part of both macros BPF_CGROUP_RUN_PROG_INET_EGRESS and
>>> BPF_CGROUP_RUN_PROG_INET_INGRESS, and it is no longer part of the
>>> function they call, __cgroup_bpf_run_filter_skb.
>>>
>>> Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>
>>
>> Acked-by: Stanislav Fomichev <sdf@google.com>
> 
> Quick question: My subject had "net:" in it. Should it have had "bpf:" in
> the subject instead?
> 
> If yes, would this warrant another version of this patch or resending it
> with a different subject?

I fixed it up with "bpf:". Applied. Thanks.
Stanislav Fomichev Feb. 13, 2024, 11:49 p.m. UTC | #4
On 02/13, Oliver Crumrine wrote:
> On Mon, Feb 12, 2024 at 08:49:14AM -0800, Stanislav Fomichev wrote:
> > On 02/09, Oliver Crumrine wrote:
> > > Originally, this patch removed a redundant check in
> > > BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
> > > the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
> > > reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
> > > and add the checks to the other macro that calls that function,
> > > BPF_CGROUP_RUN_PROG_INET_INGRESS.
> > > 
> > > To sum it up, checking that the socket exists and that it is a full
> > > socket is now part of both macros BPF_CGROUP_RUN_PROG_INET_EGRESS and
> > > BPF_CGROUP_RUN_PROG_INET_INGRESS, and it is no longer part of the
> > > function they call, __cgroup_bpf_run_filter_skb.
> > > 
> > > Signed-off-by: Oliver Crumrine <ozlinuxc@gmail.com>
> > 
> > Acked-by: Stanislav Fomichev <sdf@google.com>
> 
> Quick question: My subject had "net:" in it. Should it have had "bpf:" in
> the subject instead?
> 
> If yes, would this warrant another version of this patch or resending it
> with a different subject?
> 
> It felt right to put net: there as it felt like I was working with 
> networking code that was simply calling bpf code but I'm not exactly
> sure of that anymore.
> 
> This is my first kernel patch that has actually gone anywhere and 
> I'm just looking for some feedback as I couldn't find much good 
> documentation on kernel.org that describes how I should be doing 
> this.

It's fine, the only part that really matters is [PATCH bpf-next]. That
puts it into bpf patchwork so somebody will merge that eventually :-)

WRT documentation, Documentation/bpf/bpf_devel_QA.rst should have all
the info you need.
patchwork-bot+netdevbpf@kernel.org Feb. 13, 2024, 11:50 p.m. UTC | #5
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Fri, 9 Feb 2024 14:41:22 -0500 you wrote:
> Originally, this patch removed a redundant check in
> BPF_CGROUP_RUN_PROG_INET_EGRESS, as the check was already being done in
> the function it called, __cgroup_bpf_run_filter_skb. For v2, it was
> reccomended that I remove the check from __cgroup_bpf_run_filter_skb,
> and add the checks to the other macro that calls that function,
> BPF_CGROUP_RUN_PROG_INET_INGRESS.
> 
> [...]

Here is the summary with links:
  - [v4,bpf-next] net: remove check in __cgroup_bpf_run_filter_skb
    https://git.kernel.org/bpf/bpf-next/c/32e18e7688c6

You are awesome, thank you!
diff mbox series

Patch

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index a789266feac3..d435ad8cd4f3 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -195,8 +195,9 @@  static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
 #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb)			      \
 ({									      \
 	int __ret = 0;							      \
-	if (cgroup_bpf_enabled(CGROUP_INET_INGRESS) &&			      \
-	    cgroup_bpf_sock_enabled(sk, CGROUP_INET_INGRESS))		      \
+	if (cgroup_bpf_enabled(CGROUP_INET_INGRESS) &&			      \
+	    cgroup_bpf_sock_enabled(sk, CGROUP_INET_INGRESS) && sk &&	      \
+	    sk_fullsock(sk))						      \
 		__ret = __cgroup_bpf_run_filter_skb(sk, skb,		      \
 						    CGROUP_INET_INGRESS); \
 									      \
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 491d20038cbe..644bfb39cf9d 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1364,9 +1364,6 @@  int __cgroup_bpf_run_filter_skb(struct sock *sk,
 	struct cgroup *cgrp;
 	int ret;
 
-	if (!sk || !sk_fullsock(sk))
-		return 0;
-
 	if (sk->sk_family != AF_INET && sk->sk_family != AF_INET6)
 		return 0;