LLVM bug when storing unpacked struct?

LLVM bug when storing unpacked struct?

[Julian P Samaroo]

This is my first LKML email, so let me know if I'm doing something wrong! :)

I believe I've found a bug in LLVM's generation of BPF bytecode, and would like
to get advice on whether this is truly a bug before considering writing a
patch.

When storing an unpacked struct such as { i64, i32 } to the stack (as part of
writing a struct-typed map key), LLVM 11.0.1 generates BPF bytecode like the
following:

...
2: (b7) r1 = 2
3: (63) *(u32 *)(r10 -24) = r1
4: (b7) r1 = 4
5: (7b) *(u64 *)(r10 -32) = r1
...
8: (bf) r3 = r10
9: (07) r3 += -32
...
13: (85) call bpf_map_update_elem#2
invalid indirect read from stack off -32+12 size 16

The verifier understandably complains about this when verifying a call that
uses these stack slots, such as bpf_map_update_elem, because the associated map
definition has a key size of 16 bytes, not 12 bytes as this bytecode would
suggest. In my particular case that generated this code, my frontend doesn't
have the notion of packed structs, so I can't workaround this by making the
struct packed.

My belief is that for unpacked structs, LLVM should emit these stores as 64-bit
stores, which should be OK since the padding bytes are going to be zero (from
my limited understanding of LLVM structs). Does this seem like a reasonable
change to make? I'm also unable to test this on LLVM 12 (my language hasn't yet
updated to support that version), so this could have possibly already been
fixed; please let me know if so!

Julian

Re: LLVM bug when storing unpacked struct?

[Yonghong Song]

On 6/2/21 9:57 AM, Julian P Samaroo wrote:
> This is my first LKML email, so let me know if I'm doing something wrong! :)
> 
> I believe I've found a bug in LLVM's generation of BPF bytecode, and would like
> to get advice on whether this is truly a bug before considering writing a
> patch.
> 
> When storing an unpacked struct such as { i64, i32 } to the stack (as part of
> writing a struct-typed map key), LLVM 11.0.1 generates BPF bytecode like the
> following:
> 
> ...
> 2: (b7) r1 = 2
> 3: (63) *(u32 *)(r10 -24) = r1
> 4: (b7) r1 = 4
> 5: (7b) *(u64 *)(r10 -32) = r1
> ...
> 8: (bf) r3 = r10
> 9: (07) r3 += -32
> ...
> 13: (85) call bpf_map_update_elem#2
> invalid indirect read from stack off -32+12 size 16
> 
> The verifier understandably complains about this when verifying a call that
> uses these stack slots, such as bpf_map_update_elem, because the associated map
> definition has a key size of 16 bytes, not 12 bytes as this bytecode would
> suggest. In my particular case that generated this code, my frontend doesn't
> have the notion of packed structs, so I can't workaround this by making the
> struct packed.
> 
> My belief is that for unpacked structs, LLVM should emit these stores as 64-bit
> stores, which should be OK since the padding bytes are going to be zero (from
> my limited understanding of LLVM structs). Does this seem like a reasonable

Your assumption about padding bytes to be zero is not correct. Except 
explicitly requesting to fill padding bytes with zero e.g., using
__builtin_memset(), the compiler doesn't need to write to padding bytes.
So this is not a compiler bug.

The best approach is to do manual padding or using __builtin_memset()
before assigning values to each individual field.

> change to make? I'm also unable to test this on LLVM 12 (my language hasn't yet
> updated to support that version), so this could have possibly already been
> fixed; please let me know if so!
> 
> Julian
> 

Re: LLVM bug when storing unpacked struct?

[Julian P Samaroo]

On Wed Jun 2, 2021 at 12:19 PM CDT, Yonghong Song wrote:
>
>
> On 6/2/21 9:57 AM, Julian P Samaroo wrote:
> > This is my first LKML email, so let me know if I'm doing something wrong! :)
> > 
> > I believe I've found a bug in LLVM's generation of BPF bytecode, and would like
> > to get advice on whether this is truly a bug before considering writing a
> > patch.
> > 
> > When storing an unpacked struct such as { i64, i32 } to the stack (as part of
> > writing a struct-typed map key), LLVM 11.0.1 generates BPF bytecode like the
> > following:
> > 
> > ...
> > 2: (b7) r1 = 2
> > 3: (63) *(u32 *)(r10 -24) = r1
> > 4: (b7) r1 = 4
> > 5: (7b) *(u64 *)(r10 -32) = r1
> > ...
> > 8: (bf) r3 = r10
> > 9: (07) r3 += -32
> > ...
> > 13: (85) call bpf_map_update_elem#2
> > invalid indirect read from stack off -32+12 size 16
> > 
> > The verifier understandably complains about this when verifying a call that
> > uses these stack slots, such as bpf_map_update_elem, because the associated map
> > definition has a key size of 16 bytes, not 12 bytes as this bytecode would
> > suggest. In my particular case that generated this code, my frontend doesn't
> > have the notion of packed structs, so I can't workaround this by making the
> > struct packed.
> > 
> > My belief is that for unpacked structs, LLVM should emit these stores as 64-bit
> > stores, which should be OK since the padding bytes are going to be zero (from
> > my limited understanding of LLVM structs). Does this seem like a reasonable
>
> Your assumption about padding bytes to be zero is not correct. Except
> explicitly requesting to fill padding bytes with zero e.g., using
> __builtin_memset(), the compiler doesn't need to write to padding bytes.
> So this is not a compiler bug.
>
> The best approach is to do manual padding or using __builtin_memset()
> before assigning values to each individual field.
>

Ok, that makes sense to me! Thanks for pointing that out :)

> > change to make? I'm also unable to test this on LLVM 12 (my language hasn't yet
> > updated to support that version), so this could have possibly already been
> > fixed; please let me know if so!
> > 
> > Julian
> >