[bpf-next,v2,4/4] bpf: enable bpf_{g,s}etsockopt in BPF_CGROUP_UDP{4,6}_RECVMSG

Message ID 20210127232853.3753823-5-sdf@google.com
State New
Headers show
Series
  • Untitled series #97948
Related show

Commit Message

Stanislav Fomichev Jan. 27, 2021, 11:28 p.m.
Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on
a locked socket.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/core/filter.c                                 | 4 ++++
 tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++
 tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++
 3 files changed, 14 insertions(+)

Comments

Daniel Borkmann Jan. 29, 2021, 12:52 a.m. | #1
On 1/28/21 12:28 AM, Stanislav Fomichev wrote:
> Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on

> a locked socket.

> 

> Signed-off-by: Stanislav Fomichev <sdf@google.com>

> ---

>   net/core/filter.c                                 | 4 ++++

>   tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++

>   tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++

>   3 files changed, 14 insertions(+)

> 

> diff --git a/net/core/filter.c b/net/core/filter.c

> index ba436b1d70c2..e15d4741719a 100644

> --- a/net/core/filter.c

> +++ b/net/core/filter.c

> @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>   		case BPF_CGROUP_INET6_BIND:

>   		case BPF_CGROUP_INET4_CONNECT:

>   		case BPF_CGROUP_INET6_CONNECT:

> +		case BPF_CGROUP_UDP4_RECVMSG:

> +		case BPF_CGROUP_UDP6_RECVMSG:

>   		case BPF_CGROUP_UDP4_SENDMSG:

>   		case BPF_CGROUP_UDP6_SENDMSG:

>   		case BPF_CGROUP_INET4_GETPEERNAME:

> @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>   		case BPF_CGROUP_INET6_BIND:

>   		case BPF_CGROUP_INET4_CONNECT:

>   		case BPF_CGROUP_INET6_CONNECT:

> +		case BPF_CGROUP_UDP4_RECVMSG:

> +		case BPF_CGROUP_UDP6_RECVMSG:

>   		case BPF_CGROUP_UDP4_SENDMSG:

>   		case BPF_CGROUP_UDP6_SENDMSG:

>   		case BPF_CGROUP_INET4_GETPEERNAME:


Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one
small nit that would be good to get resolved before that. Above you now list all the attach hooks
for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type
altogether in this last commit.

Thanks,
Daniel
Stanislav Fomichev Jan. 29, 2021, 12:59 a.m. | #2
On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>

> On 1/28/21 12:28 AM, Stanislav Fomichev wrote:

> > Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on

> > a locked socket.

> >

> > Signed-off-by: Stanislav Fomichev <sdf@google.com>

> > ---

> >   net/core/filter.c                                 | 4 ++++

> >   tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++

> >   tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++

> >   3 files changed, 14 insertions(+)

> >

> > diff --git a/net/core/filter.c b/net/core/filter.c

> > index ba436b1d70c2..e15d4741719a 100644

> > --- a/net/core/filter.c

> > +++ b/net/core/filter.c

> > @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >               case BPF_CGROUP_INET6_BIND:

> >               case BPF_CGROUP_INET4_CONNECT:

> >               case BPF_CGROUP_INET6_CONNECT:

> > +             case BPF_CGROUP_UDP4_RECVMSG:

> > +             case BPF_CGROUP_UDP6_RECVMSG:

> >               case BPF_CGROUP_UDP4_SENDMSG:

> >               case BPF_CGROUP_UDP6_SENDMSG:

> >               case BPF_CGROUP_INET4_GETPEERNAME:

> > @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >               case BPF_CGROUP_INET6_BIND:

> >               case BPF_CGROUP_INET4_CONNECT:

> >               case BPF_CGROUP_INET6_CONNECT:

> > +             case BPF_CGROUP_UDP4_RECVMSG:

> > +             case BPF_CGROUP_UDP6_RECVMSG:

> >               case BPF_CGROUP_UDP4_SENDMSG:

> >               case BPF_CGROUP_UDP6_SENDMSG:

> >               case BPF_CGROUP_INET4_GETPEERNAME:

>

> Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one

> small nit that would be good to get resolved before that. Above you now list all the attach hooks

> for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type

> altogether in this last commit.

Sure, I can resend tomorrow.
But do you think it's safe and there won't ever be another sock_addr
hook that runs with an unlocked socket?
Daniel Borkmann Jan. 29, 2021, 1:08 a.m. | #3
On 1/29/21 1:59 AM, Stanislav Fomichev wrote:
> On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote:

>> On 1/28/21 12:28 AM, Stanislav Fomichev wrote:

>>> Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on

>>> a locked socket.

>>>

>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>

>>> ---

>>>    net/core/filter.c                                 | 4 ++++

>>>    tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++

>>>    tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++

>>>    3 files changed, 14 insertions(+)

>>>

>>> diff --git a/net/core/filter.c b/net/core/filter.c

>>> index ba436b1d70c2..e15d4741719a 100644

>>> --- a/net/core/filter.c

>>> +++ b/net/core/filter.c

>>> @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>>>                case BPF_CGROUP_INET6_BIND:

>>>                case BPF_CGROUP_INET4_CONNECT:

>>>                case BPF_CGROUP_INET6_CONNECT:

>>> +             case BPF_CGROUP_UDP4_RECVMSG:

>>> +             case BPF_CGROUP_UDP6_RECVMSG:

>>>                case BPF_CGROUP_UDP4_SENDMSG:

>>>                case BPF_CGROUP_UDP6_SENDMSG:

>>>                case BPF_CGROUP_INET4_GETPEERNAME:

>>> @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

>>>                case BPF_CGROUP_INET6_BIND:

>>>                case BPF_CGROUP_INET4_CONNECT:

>>>                case BPF_CGROUP_INET6_CONNECT:

>>> +             case BPF_CGROUP_UDP4_RECVMSG:

>>> +             case BPF_CGROUP_UDP6_RECVMSG:

>>>                case BPF_CGROUP_UDP4_SENDMSG:

>>>                case BPF_CGROUP_UDP6_SENDMSG:

>>>                case BPF_CGROUP_INET4_GETPEERNAME:

>>

>> Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one

>> small nit that would be good to get resolved before that. Above you now list all the attach hooks

>> for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type

>> altogether in this last commit.

> Sure, I can resend tomorrow.

> But do you think it's safe and there won't ever be another sock_addr

> hook that runs with an unlocked socket?


Ok, that rationale seems reasonable to keep the series as is. It probably makes sense to add a
small comment at least to the commit log to explain the reasoning, I can do so while applying.
So no need for v3, thanks!
Stanislav Fomichev Jan. 29, 2021, 1:14 a.m. | #4
On Thu, Jan 28, 2021 at 5:08 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>

> On 1/29/21 1:59 AM, Stanislav Fomichev wrote:

> > On Thu, Jan 28, 2021 at 4:52 PM Daniel Borkmann <daniel@iogearbox.net> wrote:

> >> On 1/28/21 12:28 AM, Stanislav Fomichev wrote:

> >>> Those hooks run as BPF_CGROUP_RUN_SA_PROG_LOCK and operate on

> >>> a locked socket.

> >>>

> >>> Signed-off-by: Stanislav Fomichev <sdf@google.com>

> >>> ---

> >>>    net/core/filter.c                                 | 4 ++++

> >>>    tools/testing/selftests/bpf/progs/recvmsg4_prog.c | 5 +++++

> >>>    tools/testing/selftests/bpf/progs/recvmsg6_prog.c | 5 +++++

> >>>    3 files changed, 14 insertions(+)

> >>>

> >>> diff --git a/net/core/filter.c b/net/core/filter.c

> >>> index ba436b1d70c2..e15d4741719a 100644

> >>> --- a/net/core/filter.c

> >>> +++ b/net/core/filter.c

> >>> @@ -7023,6 +7023,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >>>                case BPF_CGROUP_INET6_BIND:

> >>>                case BPF_CGROUP_INET4_CONNECT:

> >>>                case BPF_CGROUP_INET6_CONNECT:

> >>> +             case BPF_CGROUP_UDP4_RECVMSG:

> >>> +             case BPF_CGROUP_UDP6_RECVMSG:

> >>>                case BPF_CGROUP_UDP4_SENDMSG:

> >>>                case BPF_CGROUP_UDP6_SENDMSG:

> >>>                case BPF_CGROUP_INET4_GETPEERNAME:

> >>> @@ -7039,6 +7041,8 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)

> >>>                case BPF_CGROUP_INET6_BIND:

> >>>                case BPF_CGROUP_INET4_CONNECT:

> >>>                case BPF_CGROUP_INET6_CONNECT:

> >>> +             case BPF_CGROUP_UDP4_RECVMSG:

> >>> +             case BPF_CGROUP_UDP6_RECVMSG:

> >>>                case BPF_CGROUP_UDP4_SENDMSG:

> >>>                case BPF_CGROUP_UDP6_SENDMSG:

> >>>                case BPF_CGROUP_INET4_GETPEERNAME:

> >>

> >> Looks good overall, also thanks for adding the test cases! I was about to apply, but noticed one

> >> small nit that would be good to get resolved before that. Above you now list all the attach hooks

> >> for sock_addr ctx, so we should just remove the whole switch that tests on prog->expected_attach_type

> >> altogether in this last commit.

> > Sure, I can resend tomorrow.

> > But do you think it's safe and there won't ever be another sock_addr

> > hook that runs with an unlocked socket?

>

> Ok, that rationale seems reasonable to keep the series as is. It probably makes sense to add a

> small comment at least to the commit log to explain the reasoning, I can do so while applying.

> So no need for v3, thanks!

Sounds good, thank you!

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index ba436b1d70c2..e15d4741719a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7023,6 +7023,8 @@  sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		case BPF_CGROUP_INET6_BIND:
 		case BPF_CGROUP_INET4_CONNECT:
 		case BPF_CGROUP_INET6_CONNECT:
+		case BPF_CGROUP_UDP4_RECVMSG:
+		case BPF_CGROUP_UDP6_RECVMSG:
 		case BPF_CGROUP_UDP4_SENDMSG:
 		case BPF_CGROUP_UDP6_SENDMSG:
 		case BPF_CGROUP_INET4_GETPEERNAME:
@@ -7039,6 +7041,8 @@  sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		case BPF_CGROUP_INET6_BIND:
 		case BPF_CGROUP_INET4_CONNECT:
 		case BPF_CGROUP_INET6_CONNECT:
+		case BPF_CGROUP_UDP4_RECVMSG:
+		case BPF_CGROUP_UDP6_RECVMSG:
 		case BPF_CGROUP_UDP4_SENDMSG:
 		case BPF_CGROUP_UDP6_SENDMSG:
 		case BPF_CGROUP_INET4_GETPEERNAME:
diff --git a/tools/testing/selftests/bpf/progs/recvmsg4_prog.c b/tools/testing/selftests/bpf/progs/recvmsg4_prog.c
index fc2fe8a952fa..3d1ae8b3402f 100644
--- a/tools/testing/selftests/bpf/progs/recvmsg4_prog.c
+++ b/tools/testing/selftests/bpf/progs/recvmsg4_prog.c
@@ -8,6 +8,8 @@ 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
+#include <bpf_sockopt_helpers.h>
+
 #define SERV4_IP		0xc0a801feU /* 192.168.1.254 */
 #define SERV4_PORT		4040
 
@@ -28,6 +30,9 @@  int recvmsg4_prog(struct bpf_sock_addr *ctx)
 	if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM)
 		return 1;
 
+	if (!get_set_sk_priority(ctx))
+		return 1;
+
 	ctx->user_ip4 = bpf_htonl(SERV4_IP);
 	ctx->user_port = bpf_htons(SERV4_PORT);
 
diff --git a/tools/testing/selftests/bpf/progs/recvmsg6_prog.c b/tools/testing/selftests/bpf/progs/recvmsg6_prog.c
index 6060fd63324b..27dfb21b21b4 100644
--- a/tools/testing/selftests/bpf/progs/recvmsg6_prog.c
+++ b/tools/testing/selftests/bpf/progs/recvmsg6_prog.c
@@ -8,6 +8,8 @@ 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
+#include <bpf_sockopt_helpers.h>
+
 #define SERV6_IP_0		0xfaceb00c /* face:b00c:1234:5678::abcd */
 #define SERV6_IP_1		0x12345678
 #define SERV6_IP_2		0x00000000
@@ -31,6 +33,9 @@  int recvmsg6_prog(struct bpf_sock_addr *ctx)
 	if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM)
 		return 1;
 
+	if (!get_set_sk_priority(ctx))
+		return 1;
+
 	ctx->user_ip6[0] = bpf_htonl(SERV6_IP_0);
 	ctx->user_ip6[1] = bpf_htonl(SERV6_IP_1);
 	ctx->user_ip6[2] = bpf_htonl(SERV6_IP_2);