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

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 <[e-mail]>

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 <[e-mail]>

Acked-by: Stanislav Fomichev <[e-mail]>

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 <[e-mail]>
> 
> Acked-by: Stanislav Fomichev <[e-mail]>

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 <[e-mail]>
>>
>> Acked-by: Stanislav Fomichev <[e-mail]>
> 
> 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 <[e-mail]>
> > 
> > Acked-by: Stanislav Fomichev <[e-mail]>
> 
> 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.

[e-mail] Feb. 13, 2024, 11:50 p.m. UTC | #5

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <[e-mail]>:

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!

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;