Re: [PATCH bpf-next 4/4] bpf: allow any port in bpf_bind helper

[Andrey Ignatov <rdna@fb.com>]

Stanislav Fomichev <sdf@google.com> [Mon, 2020-05-04 10:34 -0700]:
> We want to have a tighter control on what ports we bind to in
> the BPF_CGROUP_INET{4,6}_CONNECT hooks even if it means
> connect() becomes slightly more expensive. The expensive part
> comes from the fact that we now need to call inet_csk_get_port()
> that verifies that the port is not used and allocates an entry
> in the hash table for it.

FWIW: Initially that IP_BIND_ADDRESS_NO_PORT limitation came from the
fact that on my specific use-case (mysql client making 200-500 connects
per sec to mysql server) disabling the option was making application
pretty much unusable (inet_csk_get_port was taking more time than mysql
client connect timeout == 3sec).

But I guess for some use-cases that call sys_connect not too often it
makes sense.


> Since we can't rely on "snum || !bind_address_no_port" to prevent
> us from calling POST_BIND hook anymore, let's add another bind flag
> to indicate that the call site is BPF program.
> 
> Cc: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  include/net/inet_common.h                     |   2 +
>  net/core/filter.c                             |   9 +-
>  net/ipv4/af_inet.c                            |  10 +-
>  net/ipv6/af_inet6.c                           |  12 +-
>  .../bpf/prog_tests/connect_force_port.c       | 104 ++++++++++++++++++
>  .../selftests/bpf/progs/connect_force_port4.c |  28 +++++
>  .../selftests/bpf/progs/connect_force_port6.c |  28 +++++
>  7 files changed, 177 insertions(+), 16 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/connect_force_port.c
>  create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port4.c
>  create mode 100644 tools/testing/selftests/bpf/progs/connect_force_port6.c

Documentation in include/uapi/linux/bpf.h should be updated as well
since now it states this:


 *              **AF_INET6**). Looking for a free port to bind to can be
 *              expensive, therefore binding to port is not permitted by the
 *              helper: *addr*\ **->sin_port** (or **sin6_port**, respectively)
 *              must be set to zero.

IMO it's also worth to keep a note on performance implications of
setting port to non zero.


> diff --git a/net/core/filter.c b/net/core/filter.c
> index fa9ddab5dd1f..fc5161b9ff6a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4527,29 +4527,24 @@ BPF_CALL_3(bpf_bind, struct bpf_sock_addr_kern *, ctx, struct sockaddr *, addr,
>  	struct sock *sk = ctx->sk;
>  	int err;
>  
> -	/* Binding to port can be expensive so it's prohibited in the helper.
> -	 * Only binding to IP is supported.
> -	 */
>  	err = -EINVAL;
>  	if (addr_len < offsetofend(struct sockaddr, sa_family))
>  		return err;
>  	if (addr->sa_family == AF_INET) {
>  		if (addr_len < sizeof(struct sockaddr_in))
>  			return err;
> -		if (((struct sockaddr_in *)addr)->sin_port != htons(0))
> -			return err;
>  		return __inet_bind(sk, addr, addr_len,
> +				   BIND_FROM_BPF |
>  				   BIND_FORCE_ADDRESS_NO_PORT);

Should BIND_FORCE_ADDRESS_NO_PORT be passed only if port is zero?
Passing non zero port and BIND_FORCE_ADDRESS_NO_PORT at the same time
looks confusing (even though it works).

-- 
Andrey Ignatov