Re: [PATCH bpf-next v2 01/11] bpf: Introduce BPF standard streams

[Eduard Zingerman <eddyz87@gmail.com>]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

Overall logic seems right to me, a few comments below.

[...]

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 5b25d278409b..d298746f4dcc 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1538,6 +1538,41 @@ struct btf_mod_pair {
>  
>  struct bpf_kfunc_desc_tab;
>  
> +enum bpf_stream_id {
> +	BPF_STDOUT = 1,
> +	BPF_STDERR = 2,
> +};
> +
> +struct bpf_stream_elem {
> +	struct llist_node node;
> +	int total_len;
> +	int consumed_len;
> +	char str[];
> +};
> +
> +struct bpf_stream_elem_batch {
> +	struct llist_node *node;
> +};

This type is not used anymore.

> +
> +enum {
> +	BPF_STREAM_MAX_CAPACITY = (4 * 1024U * 1024U),
> +};
> +
> +struct bpf_stream {
> +	enum bpf_stream_id stream_id;

Nit: `stream_id` is never read, as streams are identified by a position
     in the bpf_prog_aux->stream.

> +	atomic_t capacity;
> +	struct llist_head log;
> +
> +	rqspinlock_t lock;
> +	struct llist_node *backlog_head;
> +	struct llist_node *backlog_tail;

Nit: maybe add comments describing what kind of data is in the llist_{head,node}? E.g.:

	atomic_t capacity;
	struct llist_head log;		 /* list of struct bpf_stream_elem in LIFO order */

	rqspinlock_t lock;		 /* backlog_{head,tail} lock */
	struct llist_node *backlog_head; /* list of struct bpf_stream_elem in FIFO order */
	struct llist_node *backlog_tail;


> +};
> +
> +struct bpf_stream_stage {
> +	struct llist_head log;
> +	int len;
> +};
> +

[...]

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> new file mode 100644
> index 000000000000..b9e6f7a43b1b
> --- /dev/null
> +++ b/kernel/bpf/stream.c

[...]

> +int bpf_stream_stage_commit(struct bpf_stream_stage *ss, struct bpf_prog *prog,
> +			    enum bpf_stream_id stream_id)
> +{
> +	struct llist_node *list, *head, *tail;
> +	struct bpf_stream *stream;
> +	int ret;
> +
> +	stream = bpf_stream_get(stream_id, prog->aux);
> +	if (!stream)
> +		return -EINVAL;
> +
> +	ret = bpf_stream_consume_capacity(stream, ss->len);
> +	if (ret)
> +		return ret;
> +
> +	list = llist_del_all(&ss->log);
> +	head = list;
> +
> +	if (!list)
> +		return 0;
> +	while (llist_next(list)) {
> +		tail = llist_next(list);
> +		list = tail;
> +	}
> +	llist_add_batch(head, tail, &stream->log);

If `llist_next(list) == NULL` at entry `tail` is never assigned?

> +	return 0;
> +}

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d5807d2efc92..5ab8742d2c00 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13882,10 +13882,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			regs[BPF_REG_0].type = PTR_TO_BTF_ID;
>  			regs[BPF_REG_0].btf_id = ptr_type_id;
>  
> -			if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache])
> +			if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache]) {
>  				regs[BPF_REG_0].type |= PTR_UNTRUSTED;
> -
> -			if (is_iter_next_kfunc(&meta)) {
> +			} else if (is_iter_next_kfunc(&meta)) {

Nit: unrelated change?

>  				struct bpf_reg_state *cur_iter;
>  
>  				cur_iter = get_iter_from_state(env->cur_state, &meta);

[...]