Re: [bpf-next PATCH 5/7] bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data

On 01/16/2018 06:25 PM, Alexei Starovoitov wrote:

On Fri, Jan 12, 2018 at 10:11:11AM -0800, John Fastabend wrote:

quoted

This implements a BPF ULP layer to allow policy enforcement and
monitoring at the socket layer. In order to support this a new
program type BPF_PROG_TYPE_SK_MSG is used to run the policy at
the sendmsg/sendpage hook. To attach the policy to sockets a
sockmap is used with a new program attach type BPF_SK_MSG_VERDICT.

[...]

overall design looks clean. imo huge improvement from first version.

Great thanks for the review.

Few nits:

[...]

quoted

+
+static int bpf_tcp_push(struct sock *sk, struct scatterlist *sg,
+			int *sg_end, int flags, bool charge)
+{
+	int sendpage_flags = flags | MSG_SENDPAGE_NOTLAST;
+	int offset, ret = 0;
+	struct page *p;
+	size_t size;
+
+	size = sg->length;
+	offset = sg->offset;
+
+	while (1) {
+		if (sg_is_last(sg))
+			sendpage_flags = flags;
+
+		tcp_rate_check_app_limited(sk);
+		p = sg_page(sg);
+retry:
+		ret = do_tcp_sendpages(sk, p, offset, size, sendpage_flags);
+		if (ret != size) {
+			if (ret > 0) {
+				offset += ret;
+				size -= ret;
+				goto retry;
+			}
+
+			if (charge)
+				sk_mem_uncharge(sk,
+						sg->length - size - sg->offset);

should the bool argument be called 'uncharge' instead ?

Agreed that would be a better name, will update.

quoted

+
+			sg->offset = offset;
+			sg->length = size;
+			return ret;
+		}
+
+		put_page(p);
+		if (charge)
+			sk_mem_uncharge(sk, sg->length);
+		*sg_end += 1;
+		sg = sg_next(sg);
+		if (!sg)
+			break;
+
+		offset = sg->offset;
+		size = sg->length;
+	}
+
+	return 0;
+}

[...]

quoted

+static int bpf_tcp_sendmsg_do_redirect(struct scatterlist *sg, int sg_num,
+				       struct sk_msg_buff *md, int flags)
+{
+	int i, sg_curr = 0, err, free;
+	struct smap_psock *psock;
+	struct sock *sk;
+
+	rcu_read_lock();
+	sk = do_msg_redirect_map(md);
+	if (unlikely(!sk))
+		goto out_rcu;
+
+	psock = smap_psock_sk(sk);
+	if (unlikely(!psock))
+		goto out_rcu;
+
+	if (!refcount_inc_not_zero(&psock->refcnt))
+		goto out_rcu;
+
+	rcu_read_unlock();
+	lock_sock(sk);
+	err = bpf_tcp_push(sk, sg, &sg_curr, flags, false);
+	if (unlikely(err))
+		goto out;
+	release_sock(sk);
+	smap_release_sock(psock, sk);
+	return 0;
+out_rcu:
+	rcu_read_unlock();
+out:
+	for (i = sg_curr; i < sg_num; ++i) {
+		free += sg[i].length;
+		put_page(sg_page(&sg[i]));
+	}
+	return free;

erro path keeps rcu_lock and sk locked?

yep, although looks like rcu_read_unlock() is OK because its
released above the call but the

   if (unlikely(err))
	goto err

needs to be moved below the smap_release_sock(). Thanks!

quoted

+}
+

[...]

quoted

+	while (msg_data_left(msg)) {
+		int sg_curr;
+
+		if (sk->sk_err) {
+			err = sk->sk_err;
+			goto out_err;
+		}
+
+		copy = msg_data_left(msg);
+		if (!sk_stream_memory_free(sk))
+			goto wait_for_sndbuf;
+
+		/* sg_size indicates bytes already allocated and sg_num
+		 * is last sg element used. This is used when alloc_sg
+		 * partially allocates a scatterlist and then is sent
+		 * to wait for memory. In normal case (no memory pressure)
+		 * both sg_nun and sg_size are zero.
+		 */
+		copy = copy - sg_size;
+		err = sk_alloc_sg(sk, copy, sg, &sg_num, &sg_size, 0);
+		if (err) {
+			if (err != -ENOSPC)
+				goto wait_for_memory;
+			copy = sg_size;
+		}
+
+		err = memcopy_from_iter(sk, sg, sg_num, &msg->msg_iter, copy);
+		if (err < 0) {
+			free_sg(sk, sg, 0, sg_num);
+			goto out_err;
+		}
+
+		copied += copy;
+
+		/* If msg is larger than MAX_SKB_FRAGS we can send multiple
+		 * scatterlists per msg. However BPF decisions apply to the
+		 * entire msg.
+		 */
+		if (eval == __SK_NONE)
+			eval = smap_do_tx_msg(sk, psock, &md);

it seems sk_alloc_sg() will put 64k bytes into sg_data,

Yep upto 64k will be copied from msg into sg data.

but this program will see only first SG ?

Correct, to read further into the msg we would need to have a helper
or some other way to catch reads/writes past the first 4k and read
the next sg. We have the same limitation in cls_bpf.

I have started a patch on top of this series with the current working
title msg_apply_bytes(int bytes). This would let us apply a verdict to
a set number of bytes instead of the entire msg. By calling
msg_apply_bytes(data_end - data) a user who needs to read an entire msg
could do this in 4k chunks until the entire msg is passed through the
bpf prog.

and it's typically going to be one page only ?

yep

then what's the value of waiting for MAX_SKB_FRAGS ?

Its not waiting for MAX_SKB_FRAGS its simple copying up to MAX_SKB_FRAGS
pages in one call if possible. It seems better to me to run this loop
over as much data as we can.

quoted

+
+		switch (eval) {
+		case __SK_PASS:
+			sg_mark_end(sg + sg_num - 1);
+			err = bpf_tcp_push(sk, sg, &sg_curr, flags, true);
+			if (unlikely(err)) {
+				copied -= free_sg(sk, sg, sg_curr, sg_num);
+				goto out_err;
+			}
+			break;
+		case __SK_REDIRECT:
+			sg_mark_end(sg + sg_num - 1);
+			goto do_redir;
+		case __SK_DROP:
+		default:
+			copied -= free_sg(sk, sg, 0, sg_num);
+			goto out_err;
+		}

[...]

quoted

+	/* Calculate pkt data pointers and run BPF program */
+	md.data = page_address(page) + offset;
+	md.data_end = md.data + size;
+	_rc = (*prog->bpf_func)(&md, prog->insnsi);
+
+verdict:
+	rcu_read_unlock();
+	preempt_enable();
+
+	/* Moving return codes from UAPI namespace into internal namespace */
+	rc = ((_rc == SK_PASS) ? __SK_PASS : __SK_DROP);
+
+	switch (rc) {
+	case __SK_PASS:
+		lock_sock(sk);
+		rc = tcp_sendpage_locked(sk, page, offset, size, flags);
+		release_sock(sk);
+		break;
+	case __SK_REDIRECT:
+		smap_release_sock(psock, sk);
+		rc = bpf_tcp_sendpage_do_redirect(page, offset, size, flags,
+						  &md);

looks like this path wasn't tested,

:/ yep adding test now.

since above rc = ...; line cannot return REDIRECT...
probably should be common helper for both tcp_bpf_*() funcs
to call into bpf and convert rc.

will do in v2 thanks.

quoted

+		break;
+	case __SK_DROP:
+	default:
+		rc = -EACCES;
+	}
+
+	return rc;
+}
+
+static int bpf_tcp_msg_add(struct smap_psock *psock,
+			   struct sock *sk,
+			   struct bpf_prog *tx_msg)
+{
+	struct bpf_prog *orig_tx_msg;
+
+	orig_tx_msg = xchg(&psock->bpf_tx_msg, tx_msg);
+	if (orig_tx_msg)
+		bpf_prog_put(orig_tx_msg);

the function is replacing the program. why is it called bpf_tcp_msg_add ?

bad name will rename.

quoted

+
+	return tcp_set_ulp_id(sk, TCP_ULP_BPF);
+}
+

[...]

quoted

+static int bpf_tcp_ulp_register(void)
+{
+	tcp_bpf_proto = tcp_prot;
+	tcp_bpf_proto.sendmsg = bpf_tcp_sendmsg;
+	tcp_bpf_proto.sendpage = bpf_tcp_sendpage;
+	return tcp_register_ulp(&bpf_tcp_ulp_ops);

I don't see corresponding tcp_unregister_ulp().

There is none. tcp_register_ulp() adds the bpf_tcp_ulp to the list of
available ULPs and never removes it. To remove it we would have to
keep a ref count on the reg/unreg calls. This would require a couple
more patches to the ULP infra and not sure it hurts to leave the ULP
reference around...

Maybe a follow on patch? Or else it could be a patch in front of this
patch.

quoted

@@ -710,6 +1151,7 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
 	 */
 	verdict = READ_ONCE(stab->bpf_verdict);
 	parse = READ_ONCE(stab->bpf_parse);
+	tx_msg = READ_ONCE(stab->bpf_tx_msg);
 
 	if (parse && verdict) {
 		/* bpf prog refcnt may be zero if a concurrent attach operation

@@ -728,6 +1170,17 @@ static int sock_map_ctx_update_elem(struct bpf_sock_ops_kern *skops,
 		}
 	}
 
+	if (tx_msg) {
+		tx_msg = bpf_prog_inc_not_zero(stab->bpf_tx_msg);

prog_inc_not_zero() looks scary here.
Why 'not_zero' is necessary ?

Same reason we use inc_not_zero variants with the verdict/parse
programs,

 /* bpf prog refcnt may be zero if a concurrent attach operation
  * removes the program after the above READ_ONCE() but before
  * we increment the refcnt. If this is the case abort with an
  * error.
  */

quoted

+		if (IS_ERR(tx_msg)) {
+			if (verdict)
+				bpf_prog_put(verdict);
+			if (parse)
+				bpf_prog_put(parse);
+			return PTR_ERR(tx_msg);
+		}
+	}
+

Thanks!