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(-)

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;
 
-- 
2.43.0

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>

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

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.

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.