Here just skip the code(BPF_ST | BPF_NOSPEC) that has no couterpart to the loongarch.
To verify, use ltp testcase:
Without this patch:
$ ./bpf_prog02
... ...
bpf_common.c:123: TBROK: Failed verification: ??? (524)
Summary:
passed 0
failed 0
broken 1
skipped 0
warnings 0
With this patch:
$ ./bpf_prog02
... ...
Summary:
passed 0
failed 0
broken 0
skipped 0
warnings 0
Signed-off-by: George Guo <[email protected]>
---
arch/loongarch/net/bpf_jit.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 288003a9f0ca..745d344385ed 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1046,6 +1046,11 @@ static int build_body(struct jit_ctx *ctx, bool extra_pass)
if (ctx->image == NULL)
ctx->offset[i] = ctx->idx;
+ /* skip the code that has no couterpart to the host arch */
+ if(insn->code == (BPF_ST | BPF_NOSPEC)) {
+ continue;
+ }
+
ret = build_insn(insn, ctx, extra_pass);
if (ret > 0) {
i++;
--
2.34.1
On 3/26/23 6:40 AM, George Guo wrote:
> Here just skip the code(BPF_ST | BPF_NOSPEC) that has no couterpart to the loongarch.
>
> To verify, use ltp testcase:
>
> Without this patch:
> $ ./bpf_prog02
> ... ...
> bpf_common.c:123: TBROK: Failed verification: ??? (524)
>
> Summary:
> passed 0
> failed 0
> broken 1
> skipped 0
> warnings 0
>
> With this patch:
> $ ./bpf_prog02
> ... ...
> Summary:
> passed 0
> failed 0
> broken 0
> skipped 0
> warnings 0
>
> Signed-off-by: George Guo <[email protected]>
> ---
> arch/loongarch/net/bpf_jit.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 288003a9f0ca..745d344385ed 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1046,6 +1046,11 @@ static int build_body(struct jit_ctx *ctx, bool extra_pass)
> if (ctx->image == NULL)
> ctx->offset[i] = ctx->idx;
>
> + /* skip the code that has no couterpart to the host arch */
> + if(insn->code == (BPF_ST | BPF_NOSPEC)) {
> + continue;
> + }
Small nit, but could we align with other JIT implementations and place it into similar
location for consistency? Above looks a bit out of place and it should really be part
of build_insn.
diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 288003a9f0ca..d586df48ecc6 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1022,6 +1022,10 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
emit_atomic(insn, ctx);
break;
+ /* Speculation barrier */
+ case BPF_ST | BPF_NOSPEC:
+ break;
+
default:
pr_err("bpf_jit: unknown opcode %02x\n", code);
return -EINVAL;
Here just skip the opcode(BPF_ST | BPF_NOSPEC) that has no couterpart to the loongarch.
To verify, use ltp testcase:
Without this patch:
$ ./bpf_prog02
... ...
bpf_common.c:123: TBROK: Failed verification: ??? (524)
Summary:
passed 0
failed 0
broken 1
skipped 0
warnings 0
With this patch:
$ ./bpf_prog02
... ...
Summary:
passed 0
failed 0
broken 0
skipped 0
warnings 0
Signed-off-by: George Guo <[email protected]>
---
Changelog:
v2:
- place it to build_insn
- add printing for skipping bpf_jit the opcode
---
arch/loongarch/net/bpf_jit.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
index 288003a9f0ca..d3c6b1c4ccbb 100644
--- a/arch/loongarch/net/bpf_jit.c
+++ b/arch/loongarch/net/bpf_jit.c
@@ -1022,6 +1022,11 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
emit_atomic(insn, ctx);
break;
+ /* Speculation barrier */
+ case BPF_ST | BPF_NOSPEC:
+ pr_info_once("bpf_jit: skip speculation barrier opcode %0x2x\n", code);
+ break;
+
default:
pr_err("bpf_jit: unknown opcode %02x\n", code);
return -EINVAL;
--
2.34.1
On 3/28/23 9:13 AM, George Guo wrote:
> Here just skip the opcode(BPF_ST | BPF_NOSPEC) that has no couterpart to the loongarch.
>
> To verify, use ltp testcase:
>
> Without this patch:
> $ ./bpf_prog02
> ... ...
> bpf_common.c:123: TBROK: Failed verification: ??? (524)
>
> Summary:
> passed 0
> failed 0
> broken 1
> skipped 0
> warnings 0
>
> With this patch:
> $ ./bpf_prog02
> ... ...
> Summary:
> passed 0
> failed 0
> broken 0
> skipped 0
> warnings 0
>
> Signed-off-by: George Guo <[email protected]>
>
> ---
> Changelog:
> v2:
> - place it to build_insn
> - add printing for skipping bpf_jit the opcode
> ---
> arch/loongarch/net/bpf_jit.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 288003a9f0ca..d3c6b1c4ccbb 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c
> @@ -1022,6 +1022,11 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
> emit_atomic(insn, ctx);
> break;
>
> + /* Speculation barrier */
> + case BPF_ST | BPF_NOSPEC:
> + pr_info_once("bpf_jit: skip speculation barrier opcode %0x2x\n", code);
> + break;
Thanks that looks better. Question to LoongArch folks (Cc): There is no equivalent
to a speculation barrier here, correct? Either way, I think the pr_info_once() can
just be removed given there is little value for a users to have this in the kernel
log. I can take care of this while applying, that's fine.
> default:
> pr_err("bpf_jit: unknown opcode %02x\n", code);
> return -EINVAL;
>
On 2023/3/28 15:22, Daniel Borkmann wrote:
> On 3/28/23 9:13 AM, George Guo wrote:
>> Here just skip the opcode(BPF_ST | BPF_NOSPEC) that has no couterpart
>> to the loongarch.
>>
>> <snip>
>>
>> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
>> index 288003a9f0ca..d3c6b1c4ccbb 100644
>> --- a/arch/loongarch/net/bpf_jit.c
>> +++ b/arch/loongarch/net/bpf_jit.c
>> @@ -1022,6 +1022,11 @@ static int build_insn(const struct bpf_insn
>> *insn, struct jit_ctx *ctx, bool ext
>> emit_atomic(insn, ctx);
>> break;
>> + /* Speculation barrier */
>> + case BPF_ST | BPF_NOSPEC:
>> + pr_info_once("bpf_jit: skip speculation barrier opcode
>> %0x2x\n", code);
>> + break;
>
> Thanks that looks better. Question to LoongArch folks (Cc): There is no
> equivalent
> to a speculation barrier here, correct? Either way, I think the
> pr_info_once() can
> just be removed given there is little value for a users to have this in
> the kernel
> log. I can take care of this while applying, that's fine.
I can confirm there's currently no speculation barrier equivalent on
lonogarch. (Loongson says there are builtin mitigations for Spectre-V1
and V2 on their chips, and AFAIK efforts to port the exploits to
mips/loongarch have all failed a few years ago.)
And yes I'd agree with removing the warning altogether. Thanks for the
reviews!
Acked-by: WANG Xuerui <[email protected]>
>
>> default:
>> pr_err("bpf_jit: unknown opcode %02x\n", code);
>> return -EINVAL;
>>
>
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
On 3/28/23 9:52 AM, WANG Xuerui wrote:
> On 2023/3/28 15:22, Daniel Borkmann wrote:
>> On 3/28/23 9:13 AM, George Guo wrote:
>>> Here just skip the opcode(BPF_ST | BPF_NOSPEC) that has no couterpart to the loongarch.
>>>
>>> <snip>
>>>
>>> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
>>> index 288003a9f0ca..d3c6b1c4ccbb 100644
>>> --- a/arch/loongarch/net/bpf_jit.c
>>> +++ b/arch/loongarch/net/bpf_jit.c
>>> @@ -1022,6 +1022,11 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>>> emit_atomic(insn, ctx);
>>> break;
>>> + /* Speculation barrier */
>>> + case BPF_ST | BPF_NOSPEC:
>>> + pr_info_once("bpf_jit: skip speculation barrier opcode %0x2x\n", code);
>>> + break;
>>
>> Thanks that looks better. Question to LoongArch folks (Cc): There is no equivalent
>> to a speculation barrier here, correct? Either way, I think the pr_info_once() can
>> just be removed given there is little value for a users to have this in the kernel
>> log. I can take care of this while applying, that's fine.
>
> I can confirm there's currently no speculation barrier equivalent on lonogarch. (Loongson says there are builtin mitigations for Spectre-V1 and V2 on their chips, and AFAIK efforts to port the exploits to mips/loongarch have all failed a few years ago.)
>
> And yes I'd agree with removing the warning altogether. Thanks for the reviews!
>
> Acked-by: WANG Xuerui <[email protected]>
Ok, sounds good. I've cleaned this up and applied to bpf tree. Thanks!
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=a6f6a95f25803500079513780d11a911ce551d76