On Wed, 28 May 2025 at 01:47, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> 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.
>
Will drop.
> > +
> > +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.
Ack.
>
> > + 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;
>
>
Will do.
> > +};
> > +
> > +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?
The assumption is llist_del_all being non-NULL means llist_next is
going to return a non-NULL value at least once.
Does that address your concern?
>
> > + 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?
Yeah, will drop.
>
> > struct bpf_reg_state *cur_iter;
> >
> > cur_iter = get_iter_from_state(env->cur_state, &meta);
>
> [...]
Thanks!
