Re: [PATCH v7 bpf-next 09/12] libbpf: support llvm-generated indirect jumps

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 25/10/27 03:59PM, Eduard Zingerman wrote:
> On Mon, 2025-10-27 at 15:38 -0700, Eduard Zingerman wrote:
> > [...]
> >
> > > +static int create_jt_map(struct bpf_object *obj, struct bpf_program *prog, struct reloc_desc *relo)
> > > +{
> > > +	const __u32 jt_entry_size = 8;
> > > +	int sym_off = relo->sym_off;
> > > +	int jt_size = relo->sym_size;
> > > +	__u32 max_entries = jt_size / jt_entry_size;
> > > +	__u32 value_size = sizeof(struct bpf_insn_array_value);
> > > +	struct bpf_insn_array_value val = {};
> > > +	int subprog_idx;
> > > +	int map_fd, err;
> > > +	__u64 insn_off;
> > > +	__u64 *jt;
> > > +	__u32 i;
> > > +
> > > +	map_fd = find_jt_map(obj, prog, sym_off);
> > > +	if (map_fd >= 0)
> > > +		return map_fd;
> > > +
> > > +	if (sym_off % jt_entry_size) {
> > > +		pr_warn("jumptable start %d should be multiple of %u\n",
> > > +			sym_off, jt_entry_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (jt_size % jt_entry_size) {
> > > +		pr_warn("jumptable size %d should be multiple of %u\n",
> > > +			jt_size, jt_entry_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	map_fd = bpf_map_create(BPF_MAP_TYPE_INSN_ARRAY, ".jumptables",
> > > +				4, value_size, max_entries, NULL);
> > > +	if (map_fd < 0)
> > > +		return map_fd;
> > > +
> > > +	if (!obj->jumptables_data) {
> > > +		pr_warn("map '.jumptables': ELF file is missing jump table data\n");
> > > +		err = -EINVAL;
> > > +		goto err_close;
> > > +	}
> > > +	if (sym_off + jt_size > obj->jumptables_data_sz) {
> > > +		pr_warn("jumptables_data size is %zd, trying to access %d\n",
> > > +			obj->jumptables_data_sz, sym_off + jt_size);
> > > +		err = -EINVAL;
> > > +		goto err_close;
> > > +	}
> > > +
> > > +	jt = (__u64 *)(obj->jumptables_data + sym_off);
> > > +	for (i = 0; i < max_entries; i++) {
> > > +		/*
> > > +		 * The offset should be made to be relative to the beginning of
> > > +		 * the main function, not the subfunction.
> > > +		 */
> > > +		insn_off = jt[i]/sizeof(struct bpf_insn);
> > > +		if (!prog->subprogs) {
> > > +			insn_off -= prog->sec_insn_off;
> > > +		} else {
> > > +			subprog_idx = find_subprog_idx(prog, relo->insn_idx);
> >
> > Nit: find_subprog_idx(prog, relo->insn_idx) can be moved outside of the loop, I think.
> >
> > > +			if (subprog_idx < 0) {
> > > +				pr_warn("invalid jump insn idx[%d]: %d, no subprog found\n",
> > > +					i, relo->insn_idx);
> > > +				err = -EINVAL;
> > > +			}
> > > +			insn_off -= prog->subprogs[subprog_idx].sec_insn_off;
> > > +			insn_off += prog->subprogs[subprog_idx].sub_insn_off;
> > > +		}
> 
> I think I found a bug, related to this code path.
> Consider the following test case:
> 
> 	SEC("socket")
> 	__naked void foo(void)
> 	{
> 	        asm volatile ("                                         \
> 	        .pushsection .jumptables,\"\",@progbits;                \
> 	jt0_%=:                                                         \
> 	        .quad ret0_%=;                                          \
> 	        .quad ret1_%=;                                          \
> 	        .size jt0_%=, 16;                                       \
> 	        .global jt0_%=;                                         \
> 	        .popsection;                                            \
> 	                                                                \
> 	        r0 = jt0_%= ll;                                         \
> 	        r0 += 8;                                                \
> 	        r0 = *(u64 *)(r0 + 0);                                  \
> 	        .8byte %[gotox_r0];                                     \
> 	        ret0_%=:                                                \
> 	        r0 = 0;                                                 \
> 	        exit;                                                   \
> 	        ret1_%=:                                                \
> 	        r0 = 1;                                                 \
> 	        call bar;                                               \
> 	        exit;                                                   \
> 	"       :                                                       \
> 	        : __imm_insn(gotox_r0, BPF_RAW_INSN(BPF_JMP | BPF_JA | BPF_X, BPF_REG_0, 0, 0 , 0))
> 	        : __clobber_all);
> 	}
> 	
> 	__used
> 	static int bar(void)
> 	{
> 	        return 0;
> 	}
> 
> Note a call instruction referring bar().  It triggers the code path
> above (we need a test case with subprograms in verifier_gotox).
> The test case fails to load with the following error:
> 
>   libbpf: invalid jump insn idx[0]: 0, no subprog found
>   libbpf: prog 'foo': relo #0: can't create jump table: sym_off 368
>   libbpf: prog 'foo': failed to relocate data references: -EINVAL
> 
> If I remove the `call bar;`, test case loads and passes.
> 
> > > +
> > > +		/*
> > > +		 * LLVM-generated jump tables contain u64 records, however
> > > +		 * should contain values that fit in u32.
> > > +		 */
> > > +		if (insn_off > UINT32_MAX) {
> > > +			pr_warn("invalid jump table value %llx at offset %d\n",
>                                                           ^^^^
> Nit:                                              maybe add 0x prefix here?

Sure, added.

> > > +				jt[i], sym_off + i);
> > > +			err = -EINVAL;
> > > +			goto err_close;
> > > +		}
> > > +
> > > +		val.orig_off = insn_off;
> > > +		err = bpf_map_update_elem(map_fd, &i, &val, 0);
> > > +		if (err)
> > > +			goto err_close;
> > > +	}
> >
> > [...]