BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK
@ 2020-07-29  0:31 Stanislav Fomichev
  2020-07-29  0:31 ` [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release} Stanislav Fomichev
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2020-07-29  0:31 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Martin KaFai Lau

This lets us use socket storage from the following hooks:
* BPF_CGROUP_INET_SOCK_CREATE
* BPF_CGROUP_INET_SOCK_RELEASE
* BPF_CGROUP_INET4_POST_BIND
* BPF_CGROUP_INET6_POST_BIND

Using existing 'bpf_sk_storage_get_proto' doesn't work because
second argument is ARG_PTR_TO_SOCKET. Even though
BPF_PROG_TYPE_CGROUP_SOCK hooks operate on 'struct bpf_sock',
the verifier still considers it as a PTR_TO_CTX.
That's why I'm adding another 'bpf_sk_storage_get_cg_sock_proto'
definition strictly for BPF_PROG_TYPE_CGROUP_SOCK which accepts
ARG_PTR_TO_CTX which is really 'struct sock' for this program type.

Cc: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 net/core/bpf_sk_storage.c | 10 ++++++++++
 net/core/filter.c         |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index eafcd15e7dfd..d3377c90a291 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -944,6 +944,16 @@ const struct bpf_func_proto bpf_sk_storage_get_proto = {
 	.arg4_type	= ARG_ANYTHING,
 };
 
+const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto = {
+	.func		= bpf_sk_storage_get,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_MAP_VALUE_OR_NULL,
+	.arg1_type	= ARG_CONST_MAP_PTR,
+	.arg2_type	= ARG_PTR_TO_CTX, /* context is 'struct sock' */
+	.arg3_type	= ARG_PTR_TO_MAP_VALUE_OR_NULL,
+	.arg4_type	= ARG_ANYTHING,
+};
+
 const struct bpf_func_proto bpf_sk_storage_delete_proto = {
 	.func		= bpf_sk_storage_delete,
 	.gpl_only	= false,
diff --git a/net/core/filter.c b/net/core/filter.c
index 29e3455122f7..7124f0fe6974 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6187,6 +6187,7 @@ bool bpf_helper_changes_pkt_data(void *func)
 }
 
 const struct bpf_func_proto bpf_event_output_data_proto __weak;
+const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto __weak;
 
 static const struct bpf_func_proto *
 sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
@@ -6219,6 +6220,8 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_get_cgroup_classid:
 		return &bpf_get_cgroup_classid_curr_proto;
 #endif
+	case BPF_FUNC_sk_storage_get:
+		return &bpf_sk_storage_get_cg_sock_proto;
 	default:
 		return bpf_base_func_proto(func_id);
 	}
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release}
  2020-07-29  0:31 [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Stanislav Fomichev
@ 2020-07-29  0:31 ` Stanislav Fomichev
  2020-07-29  6:12   ` Song Liu
  2020-07-29  6:11 ` [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Song Liu
  2020-07-29 22:52 ` Daniel Borkmann
  2 siblings, 1 reply; 6+ messages in thread
From: Stanislav Fomichev @ 2020-07-29  0:31 UTC (permalink / raw)
  To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Martin KaFai Lau

Augment udp_limit test to set and verify socket storage value.
That should be enough to exercise the changes from the previous
patch.

Cc: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/progs/udp_limit.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/udp_limit.c b/tools/testing/selftests/bpf/progs/udp_limit.c
index 8429b22525a7..165e3c2dd9a3 100644
--- a/tools/testing/selftests/bpf/progs/udp_limit.c
+++ b/tools/testing/selftests/bpf/progs/udp_limit.c
@@ -6,14 +6,28 @@
 
 int invocations = 0, in_use = 0;
 
+struct {
+	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, int);
+} sk_map SEC(".maps");
+
 SEC("cgroup/sock_create")
 int sock(struct bpf_sock *ctx)
 {
+	int *sk_storage;
 	__u32 key;
 
 	if (ctx->type != SOCK_DGRAM)
 		return 1;
 
+	sk_storage = bpf_sk_storage_get(&sk_map, ctx, 0,
+					BPF_SK_STORAGE_GET_F_CREATE);
+	if (!sk_storage)
+		return 0;
+	*sk_storage = 0xdeadbeef;
+
 	__sync_fetch_and_add(&invocations, 1);
 
 	if (in_use > 0) {
@@ -31,11 +45,16 @@ int sock(struct bpf_sock *ctx)
 SEC("cgroup/sock_release")
 int sock_release(struct bpf_sock *ctx)
 {
+	int *sk_storage;
 	__u32 key;
 
 	if (ctx->type != SOCK_DGRAM)
 		return 1;
 
+	sk_storage = bpf_sk_storage_get(&sk_map, ctx, 0, 0);
+	if (!sk_storage || *sk_storage != 0xdeadbeef)
+		return 0;
+
 	__sync_fetch_and_add(&invocations, 1);
 	__sync_fetch_and_add(&in_use, -1);
 	return 1;
-- 
2.28.0.163.g6104cc2f0b6-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK
  2020-07-29  0:31 [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Stanislav Fomichev
  2020-07-29  0:31 ` [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release} Stanislav Fomichev
@ 2020-07-29  6:11 ` Song Liu
  2020-07-29 22:52 ` Daniel Borkmann
  2 siblings, 0 replies; 6+ messages in thread
From: Song Liu @ 2020-07-29  6:11 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S . Miller, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau

On Tue, Jul 28, 2020 at 5:31 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> This lets us use socket storage from the following hooks:
> * BPF_CGROUP_INET_SOCK_CREATE
> * BPF_CGROUP_INET_SOCK_RELEASE
> * BPF_CGROUP_INET4_POST_BIND
> * BPF_CGROUP_INET6_POST_BIND
>
> Using existing 'bpf_sk_storage_get_proto' doesn't work because
> second argument is ARG_PTR_TO_SOCKET. Even though
> BPF_PROG_TYPE_CGROUP_SOCK hooks operate on 'struct bpf_sock',
> the verifier still considers it as a PTR_TO_CTX.
> That's why I'm adding another 'bpf_sk_storage_get_cg_sock_proto'
> definition strictly for BPF_PROG_TYPE_CGROUP_SOCK which accepts
> ARG_PTR_TO_CTX which is really 'struct sock' for this program type.
>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release}
  2020-07-29  0:31 ` [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release} Stanislav Fomichev
@ 2020-07-29  6:12   ` Song Liu
  0 siblings, 0 replies; 6+ messages in thread
From: Song Liu @ 2020-07-29  6:12 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Networking, bpf, David S . Miller, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau

On Tue, Jul 28, 2020 at 5:31 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Augment udp_limit test to set and verify socket storage value.
> That should be enough to exercise the changes from the previous
> patch.
>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK
  2020-07-29  0:31 [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Stanislav Fomichev
  2020-07-29  0:31 ` [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release} Stanislav Fomichev
  2020-07-29  6:11 ` [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Song Liu
@ 2020-07-29 22:52 ` Daniel Borkmann
  2020-07-29 23:17   ` sdf
  2 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2020-07-29 22:52 UTC (permalink / raw)
  To: Stanislav Fomichev, netdev, bpf; +Cc: davem, ast, Martin KaFai Lau

On 7/29/20 2:31 AM, Stanislav Fomichev wrote:
> This lets us use socket storage from the following hooks:
> * BPF_CGROUP_INET_SOCK_CREATE
> * BPF_CGROUP_INET_SOCK_RELEASE
> * BPF_CGROUP_INET4_POST_BIND
> * BPF_CGROUP_INET6_POST_BIND
> 
> Using existing 'bpf_sk_storage_get_proto' doesn't work because
> second argument is ARG_PTR_TO_SOCKET. Even though
> BPF_PROG_TYPE_CGROUP_SOCK hooks operate on 'struct bpf_sock',
> the verifier still considers it as a PTR_TO_CTX.
> That's why I'm adding another 'bpf_sk_storage_get_cg_sock_proto'
> definition strictly for BPF_PROG_TYPE_CGROUP_SOCK which accepts
> ARG_PTR_TO_CTX which is really 'struct sock' for this program type.
> 
> Cc: Martin KaFai Lau <kafai@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Makes sense, both applied, thanks!

[...]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 29e3455122f7..7124f0fe6974 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -6187,6 +6187,7 @@ bool bpf_helper_changes_pkt_data(void *func)
>   }
>   
>   const struct bpf_func_proto bpf_event_output_data_proto __weak;
> +const struct bpf_func_proto bpf_sk_storage_get_cg_sock_proto __weak;
>   
>   static const struct bpf_func_proto *
>   sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> @@ -6219,6 +6220,8 @@ sock_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>   	case BPF_FUNC_get_cgroup_classid:
>   		return &bpf_get_cgroup_classid_curr_proto;
>   #endif
> +	case BPF_FUNC_sk_storage_get:
> +		return &bpf_sk_storage_get_cg_sock_proto;

Been wondering whether we need these for connect/sendmsg/etc hooks that operate
on sock_addr, but for those we have them already covered in sock_addr_func_proto()
therefore all good.

sock_addr_func_proto() also lists the BPF_FUNC_sk_storage_delete. Should we add
that one as well for sock_filter_func_proto()? Presumably create/release doesn't
make sense, but any use case for bind hook?

>   	default:
>   		return bpf_base_func_proto(func_id);
>   	}
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK
  2020-07-29 22:52 ` Daniel Borkmann
@ 2020-07-29 23:17   ` sdf
  0 siblings, 0 replies; 6+ messages in thread
From: sdf @ 2020-07-29 23:17 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: netdev, bpf, davem, ast, Martin KaFai Lau

On 07/30, Daniel Borkmann wrote:
> On 7/29/20 2:31 AM, Stanislav Fomichev wrote:
[..]
> sock_addr_func_proto() also lists the BPF_FUNC_sk_storage_delete. Should  
> we add
> that one as well for sock_filter_func_proto()? Presumably create/release  
> doesn't
> make sense, but any use case for bind hook?
Right, I didn't think delete makes sense for create/release, but maybe
it does for post_bind :-/
Let me put it on my list, I'll follow up!

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29  0:31 [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Stanislav Fomichev
2020-07-29  0:31 ` [PATCH bpf-next 2/2] selftests/bpf: verify socket storage in cgroup/sock_{create,release} Stanislav Fomichev
2020-07-29  6:12   ` Song Liu
2020-07-29  6:11 ` [PATCH bpf-next 1/2] bpf: expose socket storage to BPF_PROG_TYPE_CGROUP_SOCK Song Liu
2020-07-29 22:52 ` Daniel Borkmann
2020-07-29 23:17   ` sdf

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git