[PATCH bpf-next v2 0/2] Support riscv jit to provide

[PATCH bpf-next v2 0/2] Support riscv jit to provide

[Pu Lehui]

patch 1 fix an issue that could not print bpf line info due
to data inconsistency in 32-bit environment.

patch 2 add support for riscv jit to provide bpf_line_info.
Both RV32 and RV64 tests have been passed as like follow:

./test_progs -a btf
#19 btf:OK
Summary: 1/215 PASSED, 0 SKIPPED, 0 FAILED

v2:
- Remove some trivial code

v1: https://lore.kernel.org/bpf/20220426140924.3308472-1-pulehui@huawei.com

Pu Lehui (2):
  bpf: Unify data extension operation of jited_ksyms and jited_linfo
  riscv, bpf: Support riscv jit to provide bpf_line_info

 arch/riscv/net/bpf_jit.h      | 1 +
 arch/riscv/net/bpf_jit_core.c | 7 ++++++-
 kernel/bpf/syscall.c          | 5 ++++-
 3 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH bpf-next v2 1/2] bpf: Unify data extension operation of jited_ksyms and jited_linfo

[Pu Lehui]

We found that 32-bit environment can not print bpf line info due
to data inconsistency between jited_ksyms[0] and jited_linfo[0].

For example:
jited_kyms[0] = 0xb800067c, jited_linfo[0] = 0xffffffffb800067c

We know that both of them store bpf func address, but due to the
different data extension operations when extended to u64, they may
not be the same. We need to unify the data extension operations of
them.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 kernel/bpf/syscall.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e9e3e49c0eb7..18137ea5190d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3871,13 +3871,16 @@ static int bpf_prog_get_info_by_fd(struct file *file,
 		info.nr_jited_line_info = 0;
 	if (info.nr_jited_line_info && ulen) {
 		if (bpf_dump_raw_ok(file->f_cred)) {
+			unsigned long jited_linfo_addr;
 			__u64 __user *user_linfo;
 			u32 i;
 
 			user_linfo = u64_to_user_ptr(info.jited_line_info);
 			ulen = min_t(u32, info.nr_jited_line_info, ulen);
 			for (i = 0; i < ulen; i++) {
-				if (put_user((__u64)(long)prog->aux->jited_linfo[i],
+				jited_linfo_addr = (unsigned long)
+					prog->aux->jited_linfo[i];
+				if (put_user((__u64) jited_linfo_addr,
 					     &user_linfo[i]))
 					return -EFAULT;
 			}
-- 
2.25.1

RE: [PATCH bpf-next v2 1/2] bpf: Unify data extension operation of jited_ksyms and jited_linfo

[John Fastabend]

Pu Lehui wrote:
> We found that 32-bit environment can not print bpf line info due
> to data inconsistency between jited_ksyms[0] and jited_linfo[0].
> 
> For example:
> jited_kyms[0] = 0xb800067c, jited_linfo[0] = 0xffffffffb800067c
> 
> We know that both of them store bpf func address, but due to the
> different data extension operations when extended to u64, they may
> not be the same. We need to unify the data extension operations of
> them.
> 
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---
>  kernel/bpf/syscall.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index e9e3e49c0eb7..18137ea5190d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3871,13 +3871,16 @@ static int bpf_prog_get_info_by_fd(struct file *file,
>  		info.nr_jited_line_info = 0;
>  	if (info.nr_jited_line_info && ulen) {
>  		if (bpf_dump_raw_ok(file->f_cred)) {
> +			unsigned long jited_linfo_addr;
>  			__u64 __user *user_linfo;
>  			u32 i;
>  
>  			user_linfo = u64_to_user_ptr(info.jited_line_info);
>  			ulen = min_t(u32, info.nr_jited_line_info, ulen);
>  			for (i = 0; i < ulen; i++) {
> -				if (put_user((__u64)(long)prog->aux->jited_linfo[i],
> +				jited_linfo_addr = (unsigned long)
> +					prog->aux->jited_linfo[i];
> +				if (put_user((__u64) jited_linfo_addr,
>  					     &user_linfo[i]))

the logic is fine but i'm going to nitpick a bit this 4 lines is ugly
just make it slightly longer than 80chars or use a shoarter name? For
example,

			for (i = 0; i < ulen; i++) {
				unsigned long l;

				l = (unsigned long) prog->aux->jited_linfo[i];
				if (put_user((__u64) l, &user_linfo[i]))

is much nicer -- no reason to smash single assignment across multiple
lines. My $.02.

Thanks,
John

Re: [PATCH bpf-next v2 1/2] bpf: Unify data extension operation of jited_ksyms and jited_linfo

[Pu Lehui]

On 2022/5/7 4:52, John Fastabend wrote:
> Pu Lehui wrote:
>> We found that 32-bit environment can not print bpf line info due
>> to data inconsistency between jited_ksyms[0] and jited_linfo[0].
>>
>> For example:
>> jited_kyms[0] = 0xb800067c, jited_linfo[0] = 0xffffffffb800067c
>>
>> We know that both of them store bpf func address, but due to the
>> different data extension operations when extended to u64, they may
>> not be the same. We need to unify the data extension operations of
>> them.
>>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>> ---
>>   kernel/bpf/syscall.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index e9e3e49c0eb7..18137ea5190d 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -3871,13 +3871,16 @@ static int bpf_prog_get_info_by_fd(struct file *file,
>>   		info.nr_jited_line_info = 0;
>>   	if (info.nr_jited_line_info && ulen) {
>>   		if (bpf_dump_raw_ok(file->f_cred)) {
>> +			unsigned long jited_linfo_addr;
>>   			__u64 __user *user_linfo;
>>   			u32 i;
>>   
>>   			user_linfo = u64_to_user_ptr(info.jited_line_info);
>>   			ulen = min_t(u32, info.nr_jited_line_info, ulen);
>>   			for (i = 0; i < ulen; i++) {
>> -				if (put_user((__u64)(long)prog->aux->jited_linfo[i],
>> +				jited_linfo_addr = (unsigned long)
>> +					prog->aux->jited_linfo[i];
>> +				if (put_user((__u64) jited_linfo_addr,
>>   					     &user_linfo[i]))
> 
> the logic is fine but i'm going to nitpick a bit this 4 lines is ugly
> just make it slightly longer than 80chars or use a shoarter name? For
> example,
> 
> 			for (i = 0; i < ulen; i++) {
> 				unsigned long l;
> 
> 				l = (unsigned long) prog->aux->jited_linfo[i];
> 				if (put_user((__u64) l, &user_linfo[i]))
> 
> is much nicer -- no reason to smash single assignment across multiple
> lines. My $.02.
> 

Okay, It sounds good. I will make change in next version. Thanks.

> Thanks,
> John
> .
> 

[PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

[Pu Lehui]

Add support for riscv jit to provide bpf_line_info.
We need to consider the prologue offset in ctx->offset,
but unlike x86 and arm64, ctx->offset of riscv does not
provide an extra slot for the prologue, so here we just
calculate the len of prologue and add it to ctx->offset
at the end. Both RV64 and RV32 have been tested.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 arch/riscv/net/bpf_jit.h      | 1 +
 arch/riscv/net/bpf_jit_core.c | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 2a3715bf29fe..7dbbad7595f0 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -69,6 +69,7 @@ struct rv_jit_context {
 	struct bpf_prog *prog;
 	u16 *insns;		/* RV insns */
 	int ninsns;
+	int prologue_offset;
 	int epilogue_offset;
 	int *offset;		/* BPF to RV */
 	int nexentries;
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index be743d700aa7..6383eb591b0d 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	unsigned int prog_size = 0, extable_size = 0;
 	bool tmp_blinded = false, extra_pass = false;
 	struct bpf_prog *tmp, *orig_prog = prog;
-	int pass = 0, prev_ninsns = 0, i;
+	int pass = 0, prev_ninsns = 0, prologue_len, i;
 	struct rv_jit_data *jit_data;
 	struct rv_jit_context *ctx;
 
@@ -95,6 +95,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 			prog = orig_prog;
 			goto out_offset;
 		}
+		ctx->prologue_offset = ctx->ninsns;
 		bpf_jit_build_prologue(ctx);
 		ctx->epilogue_offset = ctx->ninsns;
 		bpf_jit_build_epilogue(ctx);
@@ -161,6 +162,10 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 
 	if (!prog->is_func || extra_pass) {
 		bpf_jit_binary_lock_ro(jit_data->header);
+		prologue_len = ctx->epilogue_offset - ctx->prologue_offset;
+		for (i = 0; i < prog->len; i++)
+			ctx->offset[i] = ninsns_rvoff(prologue_len + ctx->offset[i]);
+		bpf_prog_fill_jited_linfo(prog, ctx->offset);
 out_offset:
 		kfree(ctx->offset);
 		kfree(jit_data);
-- 
2.25.1

RE: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

[John Fastabend]

Pu Lehui wrote:
> Add support for riscv jit to provide bpf_line_info.
> We need to consider the prologue offset in ctx->offset,
> but unlike x86 and arm64, ctx->offset of riscv does not
> provide an extra slot for the prologue, so here we just
> calculate the len of prologue and add it to ctx->offset
> at the end. Both RV64 and RV32 have been tested.
> 
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> ---

Looks reasonable to me, but would be good if someone with RISC
knowledge takes a lookt hough.

Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

[Luke Nelson]

Thanks for the patch! I have a couple of notes written down below.

> +               ctx->prologue_offset = ctx->ninsns;
> ...
> +               prologue_len = ctx->epilogue_offset - ctx->prologue_offset;
> +               for (i = 0; i < prog->len; i++)
> +                       ctx->offset[i] = ninsns_rvoff(prologue_len + ctx->offset[i]);

The logic looks correct to me; my only nit is that the name
prologue_offset might be a bit confusing. The prologue is always at
the beginning of the final JITed program, it just happens to be that
the prologue is emitted "out of order" on the initial/internal passes
that compute offsets.

What prologue_offset really measures in your code is the length of the
body of the JITed program. What do you think about renaming
prologue_offset to something like body_len? Then the line to compute
prologue_len becomes:

        prologue_len = ctx->epilogue_offset - ctx->body_len;

This version makes more sense to me why it's correct. Curious what you think.


> +               bpf_prog_fill_jited_linfo(prog, ctx->offset);

Here's a quote from the comment that documents
bpf_prog_fill_jited_linfo in kernel/bpf/core.c:

/* The jit engine is responsible to provide an array
 * for insn_off to the jited_off mapping (insn_to_jit_off).
...
 * jited_off is the byte off to the last byte of the jited insn.

This comment says that ctx->offset (passed to this function as
insn_to_jit_off) should map each instruction to the offset of the last
byte of the JITed instructions, but as I understand it your patch sets
ctx->offset[i] to be the offset _one past_ the last byte of the JITed
instructions (i.e., the first byte of the next instruction). I'm not
sure if this is a bug in your code, in this comment, or in my
understanding :)

As a concrete example, suppose the BPF instruction at index 0 compiles
to 2 (non-compressed) RISC-V instructions, or 8 bytes. Then
ctx->offset[0] will be 2 after the initial JIT passes, and your code
would update ctx->offset[0] to be 4*prologue_len + 8. This offset
corresponds to the first byte of insns[1], not the last byte of
insn[0], which would be 4*prologue_len + 7.

My guess would be that the comment is out of date and your code is
doing the correct thing, since it seems in line with what other JITs
are doing. If that's the case, maybe we can consider updating that
comment at some point. I'm curious if the tests you ran would break if
you changed your code to match what the comment says (i.e.,
subtracting 1 byte from each element in ctx->offset before passing to
bpf_prog_fill_jited_linfo).


> ./test_progs -a btf
> #19 btf:OK
> Summary: 1/215 PASSED, 0 SKIPPED, 0 FAILED

Last, did you have a chance to run any of the other tests with your
change (e.g., test_verifier, test_bpf.ko, other tests in test_progs)?
I don't expect this change to break any tests, but may as well run
them if it's easy enough just to be sure.


Thanks!
- Luke

Re: [PATCH bpf-next v2 2/2] riscv, bpf: Support riscv jit to provide bpf_line_info

[Pu Lehui]

Hi Luke,

On 2022/5/7 5:44, Luke Nelson wrote:
> Thanks for the patch! I have a couple of notes written down below.
> 
>> +               ctx->prologue_offset = ctx->ninsns;
>> ...
>> +               prologue_len = ctx->epilogue_offset - ctx->prologue_offset;
>> +               for (i = 0; i < prog->len; i++)
>> +                       ctx->offset[i] = ninsns_rvoff(prologue_len + ctx->offset[i]);
> 
> The logic looks correct to me; my only nit is that the name
> prologue_offset might be a bit confusing. The prologue is always at
> the beginning of the final JITed program, it just happens to be that
> the prologue is emitted "out of order" on the initial/internal passes
> that compute offsets.
> 
> What prologue_offset really measures in your code is the length of the
> body of the JITed program. What do you think about renaming
> prologue_offset to something like body_len? Then the line to compute
> prologue_len becomes:
> 
>          prologue_len = ctx->epilogue_offset - ctx->body_len;
> 
> This version makes more sense to me why it's correct. Curious what you think.
>

Sorry for getting back to you so late. Thanks so much for your review. 
It seems that ctx->body_len makes more sence, I will rename it.

> 
>> +               bpf_prog_fill_jited_linfo(prog, ctx->offset);
> 
> Here's a quote from the comment that documents
> bpf_prog_fill_jited_linfo in kernel/bpf/core.c:
> 
> /* The jit engine is responsible to provide an array
>   * for insn_off to the jited_off mapping (insn_to_jit_off).
> ...
>   * jited_off is the byte off to the last byte of the jited insn.
> 
> This comment says that ctx->offset (passed to this function as
> insn_to_jit_off) should map each instruction to the offset of the last
> byte of the JITed instructions, but as I understand it your patch sets
> ctx->offset[i] to be the offset _one past_ the last byte of the JITed
> instructions (i.e., the first byte of the next instruction). I'm not
> sure if this is a bug in your code, in this comment, or in my
> understanding :)
> 
> As a concrete example, suppose the BPF instruction at index 0 compiles
> to 2 (non-compressed) RISC-V instructions, or 8 bytes. Then
> ctx->offset[0] will be 2 after the initial JIT passes, and your code
> would update ctx->offset[0] to be 4*prologue_len + 8. This offset
> corresponds to the first byte of insns[1], not the last byte of
> insn[0], which would be 4*prologue_len + 7.
> 
> My guess would be that the comment is out of date and your code is
> doing the correct thing, since it seems in line with what other JITs
> are doing. If that's the case, maybe we can consider updating that
> comment at some point. I'm curious if the tests you ran would break if
> you changed your code to match what the comment says (i.e.,
> subtracting 1 byte from each element in ctx->offset before passing to
> bpf_prog_fill_jited_linfo).
> 

IIUC,ctx->offset(passed to bpf_prog_fill_jited_linfo as insn_to_jit_off)
should be the first byte of the next instruction, or the byte off to the 
end of the current instruction.

Here's the code as below
bpf_prog_fill_jited_linfo in kernel/bpf/core.c:

	jited_linfo[i] = prog->bpf_func +
		insn_to_jit_off[linfo[i].insn_off - insn_start - 1];

we can see here that "linfo[i].insn_off - insn_start - 1" refers to the 
previous instruction, and the corresponding insn_to_jit_off refers to 
the first byte of the current instruction.

It seems the following quote might make more sense
bpf_prog_fill_jited_linfo in kernel/bpf/core.c:
* jited_off is the byte off to the "end" of the jited insn.

> 
>> ./test_progs -a btf
>> #19 btf:OK
>> Summary: 1/215 PASSED, 0 SKIPPED, 0 FAILED
> 
> Last, did you have a chance to run any of the other tests with your
> change (e.g., test_verifier, test_bpf.ko, other tests in test_progs)?
> I don't expect this change to break any tests, but may as well run
> them if it's easy enough just to be sure.
> 

Yeah, "test_verifier", "test_bpf.ko" and "test_progs -a btf" all test 
pass, as well as "test_progs" with no new failure ceses. I will attach 
the test result in v3.

Thanks,
Lehui

> 
> Thanks!
> - Luke
> .
>