Re: [PATCH v1 bpf-next 5/8] bpf: tcp: Add kfunc to adjust sk->sk_rcvlowat.

["Björn Töpel" <bjorn@kernel.org>]

Kuniyuki Iwashima <kuniyu@google.com> writes:

> We will invoke BPF SOCK_OPS prog with BPF_SOCK_OPS_RCVLOWAT_CB
> to adjust sk->sk_rcvlowat when
>
>   1. TCP stack enqueues skb to sk->sk_receive_queue
>   2. TCP recvmsg() completes
>
> Let's provide a kfunc to set sk->sk_rcvlowat.
>
> Negative values are clamped to INT_MAX, consistent with SO_RCVLOWAT.
>
> The wakeup flag is determined based on bpf_sock_ops_kern.skb:
>
>   * For the enqueue hook, skb is always non-NULL, and wakeup is
>     set to false because
>
>     * tcp_data_ready() is always called after the hooks in
>        tcp_queue_rcv() and tcp_ofo_queue().
>
>     * when tcp_fastopen_add_skb() is called for TFO SYN,
>        the socket is not yet accept()ed, and when called
>        for TFO SYN+ACK, the socket is woken up by
>        sk->sk_state_change() anyway.
>
>   * For the recvmsg() hook, skb is always NULL, and wakeup is set
>     to true because tcp_data_ready() is not called in the path.
>
> An alternative would be to support bpf_setsockopt() by adding
> BPF_SOCK_OPS_RCVLOWAT_CB to is_locked_tcp_sock_ops().
>
> However, that approach involves excessive conditionals and an
> unnecessary memcpy(), costs we do not want to pay for every skb
> in the TCP fast path.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> ---
>  net/core/filter.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 94d07a15b2ab..9c4cd27c6d4e 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12346,6 +12346,22 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
>  #endif
>  }
>  
> +__bpf_kfunc int bpf_sock_ops_tcp_set_rcvlowat(struct bpf_sock_ops_kern *skops,
> +					      int rcvlowat)
> +{
> +#ifdef CONFIG_INET
> +	if (skops->op != BPF_SOCK_OPS_RCVLOWAT_CB)
> +		return -EOPNOTSUPP;
> +
> +	if (rcvlowat < 0)
> +		rcvlowat = INT_MAX;
> +
> +	return __tcp_set_rcvlowat(skops->sk, rcvlowat, !skops->skb);
> +#else
> +	return -EOPNOTSUPP;
> +#endif
> +}
> +

(Nice work BTW! I played a bit with this, but took a sockmap/stream
parser approach instead -- this is much nicer!)

Curious if we can get into a situation that commit fcf4692fa39e ("mptcp:
prevent BPF accessing lowat from a subflow socket.") fixed?

IOW, we can construct a program that enables autolowat on MPTCP
subflows, right? If so, do we need a similar check (or checking for
subflows) in the kfunc?


Björn