netdev - RE: [Patch bpf-next v7 07/13] sock_map: introduce BPF_SK_SKB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <60623417fe3b_401fb20857@john-XPS-13-9370.notmuch>
Date:   Mon, 29 Mar 2021 13:09:59 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     bpf@...r.kernel.org, duanxiongchun@...edance.com,
        wangdongdong.6@...edance.com, jiang.wang@...edance.com,
        Cong Wang <cong.wang@...edance.com>,
        John Fastabend <john.fastabend@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Lorenz Bauer <lmb@...udflare.com>
Subject: RE: [Patch bpf-next v7 07/13] sock_map: introduce BPF_SK_SKB_VERDICT

Cong Wang wrote:
> From: Cong Wang <cong.wang@...edance.com>
> 
> Reusing BPF_SK_SKB_STREAM_VERDICT is possible but its name is
> confusing and more importantly we still want to distinguish them
> from user-space. So we can just reuse the stream verdict code but
> introduce a new type of eBPF program, skb_verdict. Users are not
> allowed to set stream_verdict and skb_verdict at the same time.
> 
> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Cc: Jakub Sitnicki <jakub@...udflare.com>
> Cc: Lorenz Bauer <lmb@...udflare.com>
> Signed-off-by: Cong Wang <cong.wang@...edance.com>
> ---

[...]

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 656eceab73bc..a045812d7c78 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -697,7 +697,7 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *psock)
>  	rcu_assign_sk_user_data(sk, NULL);
>  	if (psock->progs.stream_parser)
>  		sk_psock_stop_strp(sk, psock);
> -	else if (psock->progs.stream_verdict)
> +	else if (psock->progs.stream_verdict || psock->progs.skb_verdict)
>  		sk_psock_stop_verdict(sk, psock);
>  	write_unlock_bh(&sk->sk_callback_lock);
>  
> @@ -1024,6 +1024,8 @@ static int sk_psock_verdict_recv(read_descriptor_t *desc, struct sk_buff *skb,
>  	}
>  	skb_set_owner_r(skb, sk);
>  	prog = READ_ONCE(psock->progs.stream_verdict);
> +	if (!prog)
> +		prog = READ_ONCE(psock->progs.skb_verdict);

Trying to think through this case. User attachs skb_verdict program
to map, then updates map with a bunch of TCP sockets. The above
code will run the skb_verdict program with the TCP socket as far as
I can tell.

This is OK because there really is no difference, other than by name,
between a skb_verdict and a stream_verdict program? Do we want something
to block adding TCP sockets to maps with stream_verdict programs? It
feels a bit odd in its current state to me.

>  	if (likely(prog)) {
>  		skb_dst_drop(skb);
>  		skb_bpf_redirect_clear(skb);
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index e564fdeaada1..c46709786a49 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -155,6 +155,8 @@ static void sock_map_del_link(struct sock *sk,
>  				strp_stop = true;
>  			if (psock->saved_data_ready && stab->progs.stream_verdict)
>  				verdict_stop = true;
> +			if (psock->saved_data_ready && stab->progs.skb_verdict)
> +				verdict_stop = true;
>  			list_del(&link->list);
>  			sk_psock_free_link(link);
>  		}
> @@ -227,7 +229,7 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
>  static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
>  			 struct sock *sk)
>  {
> -	struct bpf_prog *msg_parser, *stream_parser, *stream_verdict;
> +	struct bpf_prog *msg_parser, *stream_parser, *stream_verdict, *skb_verdict;
>  	struct sk_psock *psock;
>  	int ret;
>  
> @@ -256,6 +258,15 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
>  		}
>  	}
>  
> +	skb_verdict = READ_ONCE(progs->skb_verdict);
> +	if (skb_verdict) {
> +		skb_verdict = bpf_prog_inc_not_zero(skb_verdict);
> +		if (IS_ERR(skb_verdict)) {
> +			ret = PTR_ERR(skb_verdict);
> +			goto out_put_msg_parser;
> +		}
> +	}
> +
>  	psock = sock_map_psock_get_checked(sk);
>  	if (IS_ERR(psock)) {
>  		ret = PTR_ERR(psock);
> @@ -265,6 +276,7 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
>  	if (psock) {
>  		if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) ||
>  		    (stream_parser  && READ_ONCE(psock->progs.stream_parser)) ||
> +		    (skb_verdict && READ_ONCE(psock->progs.skb_verdict)) ||
>  		    (stream_verdict && READ_ONCE(psock->progs.stream_verdict))) {
>  			sk_psock_put(sk, psock);
>  			ret = -EBUSY;

Do we need another test here,

   (skb_verdict && READ_ONCE(psock->progs.stream_verdict)

this way we return EBUSY and avoid having both stream_verdict and
skb_verdict attached on the same map?

>From commit msg:
 "Users are not allowed to set stream_verdict and skb_verdict at
  the same time."

> @@ -296,6 +308,9 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
>  	} else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
>  		psock_set_prog(&psock->progs.stream_verdict, stream_verdict);
>  		sk_psock_start_verdict(sk,psock);
> +	} else if (!stream_verdict && skb_verdict && !psock->saved_data_ready) {
> +		psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
> +		sk_psock_start_verdict(sk, psock);

Thanks,
John