Thread (9 messages) flat view 9 messages, 5 authors, 2021-01-15

Re: [RPC PATCH bpf-next] bpf: implement new BPF_CGROUP_INET_SOCK_POST_CONNECT

From: Andrii Nakryiko <hidden>
Date: 2021-01-15 02:39:34
Also in: bpf

On Thu, Jan 14, 2021 at 4:19 PM Stanislav Fomichev [off-list ref] wrote:
quoted hunk ↗ jump to hunk
We are playing with doing hybrid conntrack where BPF generates
connect/disconnect/etc events and puts them into perfbuf (or, later,
new ringbuf). We can get most of the functionality out of
existing hooks:
- BPF_CGROUP_SOCK_OPS fully covers TCP
- BPF_CGROUP_UDP4_SENDMSG covers unconnected UDP (with sampling, etc)

The only missing bit is connected UDP where we can get some
information from the existing BPF_CGROUP_INET{4,6}_CONNECT if the caller
did explicit bind(); otherwise, in an autobind case, we get
only destination addr/port and no source port because this hook
triggers prior to that.

We'd really like to avoid the cost of BPF_CGROUP_INET_EGRESS
and filtering UDP (which covers both connected and unconnected UDP,
but loses that connect/disconnect pseudo signal).

The proposal is to add a new BPF_CGROUP_INET_SOCK_POST_CONNECT which
triggers right before sys_connect exits in the AF_INET{,6} case.
The context is bpf_sock which lets BPF examine the socket state.
There is really no reason for it to trigger for all inet socks,
I've considered adding BPF_CGROUP_UDP_POST_CONNECT, but decided
that it might be better to have a generic inet case.

New hook triggers right before sys_connect() returns and gives
BPF an opportunity to explore source & destination addresses
as well as ability to return EPERM to the user.

This is somewhat analogous to the existing BPF_CGROUP_INET{4,6}_POST_BIND
hooks with the intention to log the connection addresses (after autobind).

Signed-off-by: Stanislav Fomichev <redacted>
Change-Id: I46d0122f93c58b17bfae5ba5040b0b0343908c19
---
 include/linux/bpf-cgroup.h | 17 +++++++++++++++++
 include/uapi/linux/bpf.h   |  1 +
 kernel/bpf/syscall.c       |  3 +++
 net/core/filter.c          |  4 ++++
 net/ipv4/af_inet.c         |  7 ++++++-
 5 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index 72e69a0e1e8c..f110935258b9 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -213,12 +213,29 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
        __ret;                                                                 \
 })

+#define BPF_CGROUP_RUN_SK_PROG_LOCKED(sk, type)                                       \
+({                                                                            \
+       int __ret = 0;                                                         \
+       if (cgroup_bpf_enabled) {                                              \
+               lock_sock(sk);                                                 \
+               __ret = __cgroup_bpf_run_filter_sk(sk, type);                  \
+               release_sock(sk);                                              \
+       }                                                                      \
+       __ret;                                                                 \
+})
+
 #define BPF_CGROUP_RUN_PROG_INET_SOCK(sk)                                     \
        BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET_SOCK_CREATE)

 #define BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk)                             \
        BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET_SOCK_RELEASE)

+#define BPF_CGROUP_RUN_PROG_INET_SOCK_POST_CONNECT(sk)                        \
+       BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET_SOCK_POST_CONNECT)
+
+#define BPF_CGROUP_RUN_PROG_INET_SOCK_POST_CONNECT_LOCKED(sk)                 \
+       BPF_CGROUP_RUN_SK_PROG_LOCKED(sk, BPF_CGROUP_INET_SOCK_POST_CONNECT)
+
 #define BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk)                                       \
        BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET4_POST_BIND)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a1ad32456f89..3235f7bd131f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -241,6 +241,7 @@ enum bpf_attach_type {
        BPF_XDP_CPUMAP,
        BPF_SK_LOOKUP,
        BPF_XDP,
+       BPF_CGROUP_INET_SOCK_POST_CONNECT,
Adding new bpf_attach_type enums keeps blowing up the size of struct
cgroup_bpf. Right now we have 38 different values, of which 15 values
are not related to cgroups (judging by their name). That results in 15
* (8 + 16 + 4) = 420 extra bytes wasted for each struct cgroup_bpf
(and thus struct cgroup). Probably not critical, but it would be nice
to not waste space unnecessarily.

Would anyone be interested in addressing this? Basically, instead of
using MAX_BPF_ATTACH_TYPE from enum bpf_attach_type, we'd need to have
cgroup-specific enumeration and mapping bpf_attach_type to that
bpf_cgroup_attach_type to compactly store information in struct
cgroup_bpf. Thoughts?

quoted hunk ↗ jump to hunk
        __MAX_BPF_ATTACH_TYPE
 };
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c3bb03c8371f..7d6fd1e32d22 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1958,6 +1958,7 @@ bpf_prog_load_check_attach(enum bpf_prog_type prog_type,
                switch (expected_attach_type) {
                case BPF_CGROUP_INET_SOCK_CREATE:
                case BPF_CGROUP_INET_SOCK_RELEASE:
+               case BPF_CGROUP_INET_SOCK_POST_CONNECT:
                case BPF_CGROUP_INET4_POST_BIND:
                case BPF_CGROUP_INET6_POST_BIND:
                        return 0;
@@ -2910,6 +2911,7 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
                return BPF_PROG_TYPE_CGROUP_SKB;
        case BPF_CGROUP_INET_SOCK_CREATE:
        case BPF_CGROUP_INET_SOCK_RELEASE:
+       case BPF_CGROUP_INET_SOCK_POST_CONNECT:
        case BPF_CGROUP_INET4_POST_BIND:
        case BPF_CGROUP_INET6_POST_BIND:
                return BPF_PROG_TYPE_CGROUP_SOCK;
@@ -3063,6 +3065,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
        case BPF_CGROUP_INET_EGRESS:
        case BPF_CGROUP_INET_SOCK_CREATE:
        case BPF_CGROUP_INET_SOCK_RELEASE:
+       case BPF_CGROUP_INET_SOCK_POST_CONNECT:
        case BPF_CGROUP_INET4_BIND:
        case BPF_CGROUP_INET6_BIND:
        case BPF_CGROUP_INET4_POST_BIND:
diff --git a/net/core/filter.c b/net/core/filter.c
index 9ab94e90d660..d955321d3415 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7683,12 +7683,14 @@ static bool __sock_filter_check_attach_type(int off,
                switch (attach_type) {
                case BPF_CGROUP_INET_SOCK_CREATE:
                case BPF_CGROUP_INET_SOCK_RELEASE:
+               case BPF_CGROUP_INET_SOCK_POST_CONNECT:
                        goto full_access;
                default:
                        return false;
                }
        case bpf_ctx_range(struct bpf_sock, src_ip4):
                switch (attach_type) {
+               case BPF_CGROUP_INET_SOCK_POST_CONNECT:
                case BPF_CGROUP_INET4_POST_BIND:
                        goto read_only;
                default:
@@ -7696,6 +7698,7 @@ static bool __sock_filter_check_attach_type(int off,
                }
        case bpf_ctx_range_till(struct bpf_sock, src_ip6[0], src_ip6[3]):
                switch (attach_type) {
+               case BPF_CGROUP_INET_SOCK_POST_CONNECT:
                case BPF_CGROUP_INET6_POST_BIND:
                        goto read_only;
                default:
@@ -7703,6 +7706,7 @@ static bool __sock_filter_check_attach_type(int off,
                }
        case bpf_ctx_range(struct bpf_sock, src_port):
                switch (attach_type) {
+               case BPF_CGROUP_INET_SOCK_POST_CONNECT:
                case BPF_CGROUP_INET4_POST_BIND:
                case BPF_CGROUP_INET6_POST_BIND:
                        goto read_only;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index b94fa8eb831b..568654cafa48 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -574,7 +574,10 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,

        if (!inet_sk(sk)->inet_num && inet_autobind(sk))
                return -EAGAIN;
-       return sk->sk_prot->connect(sk, uaddr, addr_len);
+       err = sk->sk_prot->connect(sk, uaddr, addr_len);
+       if (!err)
+               err = BPF_CGROUP_RUN_PROG_INET_SOCK_POST_CONNECT_LOCKED(sk);
+       return err;
 }
 EXPORT_SYMBOL(inet_dgram_connect);
Have you tried attaching the fexit program to inet_dgram_connect?
Doesn't it give all the information you need?
quoted hunk ↗ jump to hunk
@@ -723,6 +726,8 @@ int inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,

        lock_sock(sock->sk);
        err = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
Similarly here, attaching fexit to __inet_stream_connect would execute
your BPF program at exactly the same time (and then you can check for
err value).

Or the point here is to have a more "stable" BPF program type?
+       if (!err)
+               err = BPF_CGROUP_RUN_PROG_INET_SOCK_POST_CONNECT(sock->sk);
        release_sock(sock->sk);
        return err;
 }
--
2.30.0.284.gd98b1dd5eaa7-goog
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help