This is an archive of the discontinued LLVM Phabricator instance.

[BPF] Mark FI_ri as isPseudo to avoid assertion during disassembly
ClosedPublic

Authored by eddyz87 on May 8 2022, 6:52 AM.

Details

Summary

When a specific sequence of bytes is present in the file during
disassembly the disassembler fails with the following assertion:

...
     0:	18 20 00 00 00 00 00 00	lea
... Assertion `idx < size()' failed.
...
llvm::SmallVectorTemplateCommon<...>::operator[](...) ...
llvm::MCInst::getOperand(unsigned int) ...
llvm::BPFInstPrinter::printOperand(...) ...
llvm::BPFInstPrinter::printInstruction() ...
llvm::BPFInstPrinter::printInst(...) ...
...

The byte sequence causing the error is (little endian):

18 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00

The issue could be reproduced using the program bellow:

test.ir:

@G = constant
       [16 x i8]
       [i8 u0x18, i8 u0x20, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00,
        i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00, i8 u0x00],
       section "foo", align 8

Compiled and disassembled as follows:

cat test.ir | llc -march=bpfel -filetype=obj -o - \
            | llvm-objdump --arch=bpfel --section=foo -d -

This byte sequence corresponds to FI_ri instruction declared in the
BPFInstrInfo.td as follows:

def FI_ri
    : TYPE_LD_ST<BPF_IMM.Value, BPF_DW.Value,
                 (outs GPR:$dst),
                 (ins MEMri:$addr),
                 "lea\t$dst, $addr",
                 [(set i64:$dst, FIri:$addr)]> {
  // This is a tentative instruction, and will be replaced
  // with MOV_rr and ADD_ri in PEI phase
  let Inst{51-48} = 0;
  let Inst{55-52} = 2;
  let Inst{47-32} = 0;
  let Inst{31-0} = 0;
  let BPFClass = BPF_LD;
}

Notes:

  • First byte (opcode) is formed as follows:
    • BPF_IMM.Value is 0x00
    • BPF_DW.Value is 0x18
    • BPF_LD is 0x00
  • Second byte (registers) is formed as follows:
    • let Inst{55-52} = 2;
    • let Inst{51-48} = 0;

The FI_ri instruction is always replaced by MOV_rr ADD_ri instructions
pair in the BPFRegisterInfo::eliminateFrameIndex method. Thus, this
instruction should be invisible to disassembler. This patch achieves
this by adding "isPseudo" flag for this instruction.

The bug was found by decompiling of one of the BPF tests from Linux
kernel (llvm-objdump -D tools/testing/selftests/bpf/bpf_iter_sockmap.o)

Diff Detail

Event Timeline

eddyz87 created this revision.May 8 2022, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2022, 6:52 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
eddyz87 updated this revision to Diff 427932.May 8 2022, 6:57 AM
eddyz87 edited the summary of this revision. (Show Details)

Commit message fixes for better formatting

eddyz87 added a reviewer: ast.May 8 2022, 7:14 AM
eddyz87 updated this revision to Diff 427936.May 8 2022, 7:26 AM
This comment was removed by eddyz87.
eddyz87 published this revision for review.May 8 2022, 11:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2022, 11:23 AM

The following tests are passing with this patch:

  • LLVM unit and regression tests (ninja check-all)
  • BPF selftests (linux/tools/testing/selftests/bpf/vmtest.sh, bpf-next kernel repository, commit 20b87e7c29df)
yonghong-song accepted this revision.May 10 2022, 7:49 AM
yonghong-song added a subscriber: yonghong-song.

The change looks good to me. Do you have write permission to merge into llvm-project main repo? If not, I can do the merge for you.

This revision is now accepted and ready to land.May 10 2022, 7:49 AM

The change looks good to me. Do you have write permission to merge into llvm-project main repo? If not, I can do the merge for you.

Thank you,
I don't have the write permission, please merge it for me.

Could you provide proper "<Name> <Email Address>" so I can change author to you when I push the commit to the main branch?

Could you provide proper "<Name> <Email Address>" so I can change author to you when I push the commit to the main branch?

Eduard Zingerman eddyz87@gmail.com

ast added a comment.May 10 2022, 5:20 PM

a test got lost?

a test got lost?

My fault. Forgot to git add the file before the push. Will push the test shortly with a separate commit.