> On Feb 24, 2023, at 2:35 PM, Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> On 02/23, Aditi Ghag wrote:
>> The socket destroy kfunc is used to forcefully terminate sockets from
>> certain BPF contexts. We plan to use the capability in Cilium to force
>> client sockets to reconnect when their remote load-balancing backends are
>> deleted. The other use case is on-the-fly policy enforcement where existing
>> socket connections prevented by policies need to be forcefully terminated.
>> The helper allows terminating sockets that may or may not be actively
>> sending traffic.
>
>> The helper is currently exposed to certain BPF iterators where users can
>> filter, and terminate selected sockets. Additionally, the helper can only
>> be called from these BPF contexts that ensure socket locking in order to
>> allow synchronous execution of destroy helpers that also acquire socket
>> locks. The previous commit that batches UDP sockets during iteration
>> facilitated a synchronous invocation of the destroy helper from BPF context
>> by skipping taking socket locks in the destroy handler. TCP iterators
>> already supported batching.
>
>> The helper takes `sock_common` type argument, even though it expects, and
>> casts them to a `sock` pointer. This enables the verifier to allow the
>> sock_destroy kfunc to be called for TCP with `sock_common` and UDP with
>> `sock` structs. As a comparison, BPF helpers enable this behavior with the
>> `ARG_PTR_TO_BTF_ID_SOCK_COMMON` argument type. However, there is no such
>> option available with the verifier logic that handles kfuncs where BTF
>> types are inferred. Furthermore, as `sock_common` only has a subset of
>> certain fields of `sock`, casting pointer to the latter type might not
>> always be safe. Hence, the BPF kfunc converts the argument to a full sock
>> before casting.
>
>> Signed-off-by: Aditi Ghag <aditi.ghag@xxxxxxxxxxxxx>
>> ---
>> net/core/filter.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>> net/ipv4/tcp.c | 17 ++++++++++-----
>> net/ipv4/udp.c | 7 ++++--
>> 3 files changed, 72 insertions(+), 7 deletions(-)
>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 1d6f165923bf..79cd91ba13d0 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -11621,3 +11621,58 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id)
>
>> return func;
>> }
>> +
>> +/* Disables missing prototype warnings */
>> +__diag_push();
>> +__diag_ignore_all("-Wmissing-prototypes",
>> + "Global functions as their definitions will be in vmlinux BTF");
>> +
>> +/* bpf_sock_destroy: Destroy the given socket with ECONNABORTED error code.
>> + *
>> + * The helper expects a non-NULL pointer to a full socket. It invokes
>> + * the protocol specific socket destroy handlers.
>> + *
>> + * The helper can only be called from BPF contexts that have acquired the socket
>> + * locks.
>> + *
>> + * Parameters:
>> + * @sock: Pointer to socket to be destroyed
>> + *
>> + * Return:
>> + * On error, may return EPROTONOSUPPORT, EINVAL.
>> + * EPROTONOSUPPORT if protocol specific destroy handler is not implemented.
>> + * 0 otherwise
>> + */
>> +int bpf_sock_destroy(struct sock_common *sock)
>
> Prefix with __bpf_kfunc (see other kfuncs).
Will do!
>
>> +{
>> + /* Validates the socket can be type casted to a full socket. */
>> + struct sock *sk = sk_to_full_sk((struct sock *)sock);
>> +
>> + if (!sk)
>> + return -EINVAL;
>> +
>> + /* The locking semantics that allow for synchronous execution of the
>> + * destroy handlers are only supported for TCP and UDP.
>> + */
>> + if (!sk->sk_prot->diag_destroy || sk->sk_protocol == IPPROTO_RAW)
>> + return -EOPNOTSUPP;
>> +
>> + return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
>> +}
>> +
>> +__diag_pop()
>> +
>> +BTF_SET8_START(sock_destroy_kfunc_set)
>> +BTF_ID_FLAGS(func, bpf_sock_destroy)
>> +BTF_SET8_END(sock_destroy_kfunc_set)
>> +
>> +static const struct btf_kfunc_id_set bpf_sock_destroy_kfunc_set = {
>> + .owner = THIS_MODULE,
>> + .set = &sock_destroy_kfunc_set,
>> +};
>> +
>> +static int init_subsystem(void)
>> +{
>> + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_sock_destroy_kfunc_set);
>
> Is it safe? Does it mean I can call bpf_sock_destroy from any tracing
> program from anywhere? What if the socket is not locked?
> IOW, do we have to constrain it to the iterator programs (at least for
> now)?
Given kprobes are not considered as part of BPF_PROG_TYPE_TRACING, I'm not sure if there are other tracing programs with sock/sock_common arguments. Regardless, this is a valid point. I had brought up a similar topic earlier during the v1 discussion - https://lore.kernel.org/bpf/78E434B0-06A9-466F-A061-B9A05DC6DE6D@xxxxxxxxxxxxx/. I suppose you would have a similar problem in the case of setsockopt* helpers.
Is the general topic of limiting access for kfunc to a subset of BPF_PROG_* programs being discussed?
>
>> +}
>> +late_initcall(init_subsystem);
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 33f559f491c8..8123c264d8ea 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -4678,8 +4678,10 @@ int tcp_abort(struct sock *sk, int err)
>> return 0;
>> }
>
>> - /* Don't race with userspace socket closes such as tcp_close. */
>> - lock_sock(sk);
>> + /* BPF context ensures sock locking. */
>> + if (!has_current_bpf_ctx())
>> + /* Don't race with userspace socket closes such as tcp_close. */
>> + lock_sock(sk);
>
>> if (sk->sk_state == TCP_LISTEN) {
>> tcp_set_state(sk, TCP_CLOSE);
>> @@ -4688,7 +4690,9 @@ int tcp_abort(struct sock *sk, int err)
>
>> /* Don't race with BH socket closes such as inet_csk_listen_stop. */
>> local_bh_disable();
>> - bh_lock_sock(sk);
>> + if (!has_current_bpf_ctx())
>> + bh_lock_sock(sk);
>> +
>
>> if (!sock_flag(sk, SOCK_DEAD)) {
>> sk->sk_err = err;
>> @@ -4700,10 +4704,13 @@ int tcp_abort(struct sock *sk, int err)
>> tcp_done(sk);
>> }
>
>> - bh_unlock_sock(sk);
>> + if (!has_current_bpf_ctx())
>> + bh_unlock_sock(sk);
>> +
>> local_bh_enable();
>> tcp_write_queue_purge(sk);
>> - release_sock(sk);
>> + if (!has_current_bpf_ctx())
>> + release_sock(sk);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(tcp_abort);
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index 2f3978de45f2..1bc9ad92c3d4 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2925,7 +2925,9 @@ EXPORT_SYMBOL(udp_poll);
>
>> int udp_abort(struct sock *sk, int err)
>> {
>> - lock_sock(sk);
>> + /* BPF context ensures sock locking. */
>> + if (!has_current_bpf_ctx())
>> + lock_sock(sk);
>
>> /* udp{v6}_destroy_sock() sets it under the sk lock, avoid racing
>> * with close()
>> @@ -2938,7 +2940,8 @@ int udp_abort(struct sock *sk, int err)
>> __udp_disconnect(sk, 0);
>
>> out:
>> - release_sock(sk);
>> + if (!has_current_bpf_ctx())
>> + release_sock(sk);
>
>> return 0;
>> }
>> --
>> 2.34.1
