On Thu, Jul 29, 2021 at 4:56 AM Yonghong Song <yhs@xxxxxx> wrote:
> > +static __init int prepare_tail_call_tests(struct bpf_array **pprogs)
> > +{
> > + struct bpf_array *progs;
> > + int ntests = ARRAY_SIZE(tail_call_tests);
> > + int which, err;
>
> reverse christmas tree?
Will do.
> > +
> > + /* Allocate the table of programs to be used for tall calls */
> > + progs = kzalloc(sizeof(*progs) + (ntests + 1) * sizeof(progs->ptrs[0]),
> > + GFP_KERNEL);
> > + if (!progs)
> > + goto out_nomem;
> > +
> > + /* Create all eBPF programs and populate the table */
> > + for (which = 0; which < ntests; which++) {
> > + struct tail_call_test *test = &tail_call_tests[which];
> > + struct bpf_prog *fp;
> > + int len, i;
> > +
> > + /* Compute the number of program instructions */
> > + for (len = 0; len < MAX_INSNS; len++) {
> > + struct bpf_insn *insn = &test->insns[len];
> > +
> > + if (len < MAX_INSNS - 1 &&
> > + insn->code == (BPF_LD | BPF_DW | BPF_IMM))
> > + len++;
> > + if (insn->code == 0)
> > + break;
> > + }
> > +
> > + /* Allocate and initialize the program */
> > + fp = bpf_prog_alloc(bpf_prog_size(len), 0);
> > + if (!fp)
> > + goto out_nomem;
> > +
> > + fp->len = len;
> > + fp->type = BPF_PROG_TYPE_SOCKET_FILTER;
> > + fp->aux->stack_depth = test->stack_depth;
> > + memcpy(fp->insnsi, test->insns, len * sizeof(struct bpf_insn));
> > +
> > + /* Relocate runtime tail call offsets and addresses */
> > + for (i = 0; i < len; i++) {
> > + struct bpf_insn *insn = &fp->insnsi[i];
> > + int target;
> > +
> > + if (insn->imm != TAIL_CALL_MARKER)
> > + continue;
> > +
> > + switch (insn->code) {
> > + case BPF_LD | BPF_DW | BPF_IMM:
> > + if (insn->dst_reg == R2) {
>
> Looks like the above condition is not needed. It is always true.
>
> > + insn[0].imm = (u32)(long)progs;
> > + insn[1].imm = ((u64)(long)progs) >> 32;
> > + }
> > + break;
> > +
> > + case BPF_ALU | BPF_MOV | BPF_K:
> > + case BPF_ALU64 | BPF_MOV | BPF_K:
>
> case BPF_ALU64 | BPF_MOV | BPF_K is not needed.
>
> > + if (insn->off == TAIL_CALL_NULL)
> > + target = ntests;
> > + else
> > + target = which + insn->off;
> > + if (insn->dst_reg == R3)
>
> the same here, insn->dst_reg == R3 is not needed. It is always true.
I added the register checks to further restrict the cases when
rewriting is done, but it might be more clear if the instruction is
always rewritten whenever the tail call marker is set. I can remove
the unnecessary conditions.
> I suggest to set insn->off = 0. Otherwise, it is an illegal insn.
> We won't issue here because we didn't invoke verifier. It is still
> good to make the insn legel.
I agree. Fixing it.
