Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unable to run cilium with clang-4.0 #310

Closed
aanm opened this issue Mar 14, 2017 · 9 comments · Fixed by #398
Closed

Unable to run cilium with clang-4.0 #310

aanm opened this issue Mar 14, 2017 · 9 comments · Fixed by #398
Assignees
@tgraf
Labels
kind/bug This is a bug in the Cilium logic.
Milestone
0.8 release

Comments

@aanm
Copy link
Member

aanm commented Mar 14, 2017

Steps to reproduce:

  1. Download and install clang-4.0 for ubuntu 16.04 x86_64.
  2. make
  3. See errors:
In file included from bpf_lxc.c:41:
./lib/lb.h:167:33: error: taking address of packed member 'address' of class or structure 'lb6_reverse_nat' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                ipv6_addr_copy(&tuple->addr, &nat->address);
                                              ^~~~~~~~~~~~
./lib/lb.h:173:25: error: taking address of packed member 'address' of class or structure 'lb6_reverse_nat' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                ipv6_addr_copy(&tmp, &nat->address);
                                      ^~~~~~~~~~~~
./lib/lb.h:230:18: error: taking address of packed member 'address' of class or structure 'lb6_key' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        ipv6_addr_copy(&key->address, &tuple->addr);
                        ^~~~~~~~~~~~
./lib/lb.h:234:55: error: taking address of packed member 'dport' of class or structure 'lb6_key' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        return extract_l4_port(skb, tuple->nexthdr, l4_off, &key->dport);
                                                             ^~~~~~~~~~
./lib/lb.h:326:32: error: taking address of packed member 'target' of class or structure 'lb6_service' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        ipv6_addr_copy(&tuple->addr, &svc->target);
                                      ^~~~~~~~~~~
./lib/lb.h:448:55: error: taking address of packed member 'dport' of class or structure 'lb4_key' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
        return extract_l4_port(skb, tuple->nexthdr, l4_off, &key->dport);
                                                             ^~~~~~~~~~
@aanm aanm added the kind/bug This is a bug in the Cilium logic. label Mar 14, 2017
@tgraf tgraf assigned tgraf and borkmann and unassigned tgraf Mar 14, 2017
@borkmann
Copy link
Member

I'm currently running with llvm 5.0 (git checkout) and see the same thing. I checked the verifier and access to regs/stack is enforced to be aligned on kernel side. the only thing that may be unaligned if the architecture supports efficient unaligned access is the direct packet access and map value access (I think I just spotted a bug in the latter, though). So having llvm spill these warnings is not very helpful and I'll send a patch to silence them.

@borkmann borkmann mentioned this issue Mar 15, 2017
Merged
borkmann added a commit that referenced this issue Mar 15, 2017
@borkmann
bpf: disable -Waddress-of-packed-member on newer clang versions
d1eae7d 
clang 4.0 and higher bails out when compiling cilium's eBPF programs
with the following error:

  [...]
  In file included from bpf_lxc.c:41:
  ./lib/lb.h:167:33: error: taking address of packed member 'address'
  of class or structure 'lb6_reverse_nat' may result in an unaligned
  pointer value [-Werror,-Waddress-of-packed-member]
      ipv6_addr_copy(&tuple->addr, &nat->address);
                                   ^~~~~~~~~~~~
  [...]

Disable the new -Waddress-of-packed-member warning as the verifier
will eventually make the call whether a given access is fine or not.
F.e., context, stack, (regs) can never be accessed unaligned, direct
packet access and dynamic map value access may be accessed unaligned
if the underlying architecture supports it, f.e. x86/arm64. Thus,
just disable -Waddress-of-packed-member and make sure that earlier
clang versions don't bail out on not supporting the warning by adding
-Wno-unknown-warning-option to the flags.

Closes #310

Reported-by: André Martins <andre@cilium.io>
Signed-off-by: Daniel Borkmann <daniel@cilium.io>
@aanm aanm closed this as completed in #315 Mar 15, 2017
aanm pushed a commit that referenced this issue Mar 15, 2017
@borkmann @aanm
bpf: disable -Waddress-of-packed-member on newer clang versions
ca0038e 
clang 4.0 and higher bails out when compiling cilium's eBPF programs
with the following error:

  [...]
  In file included from bpf_lxc.c:41:
  ./lib/lb.h:167:33: error: taking address of packed member 'address'
  of class or structure 'lb6_reverse_nat' may result in an unaligned
  pointer value [-Werror,-Waddress-of-packed-member]
      ipv6_addr_copy(&tuple->addr, &nat->address);
                                   ^~~~~~~~~~~~
  [...]

Disable the new -Waddress-of-packed-member warning as the verifier
will eventually make the call whether a given access is fine or not.
F.e., context, stack, (regs) can never be accessed unaligned, direct
packet access and dynamic map value access may be accessed unaligned
if the underlying architecture supports it, f.e. x86/arm64. Thus,
just disable -Waddress-of-packed-member and make sure that earlier
clang versions don't bail out on not supporting the warning by adding
-Wno-unknown-warning-option to the flags.

Closes #310

Reported-by: André Martins <andre@cilium.io>
Signed-off-by: Daniel Borkmann <daniel@cilium.io>
@aanm
Copy link
Member Author

aanm commented Mar 15, 2017 •
edited

@borkmann I'm going to reopen this since the issue still occurs because of CLANG_OPTS in bpf/join_ep.sh, can you take care of that as well?

@aanm aanm reopened this Mar 15, 2017
@borkmann
Copy link
Member

Indeed, it looks like we have it in total also in 4 other locations (init.sh, join_ep.sh, run_probes.sh, 06-lb.sh). I'll send a follow-up.

@borkmann
Copy link
Member

For the log, see latest comment on the PR after the -Waddress-of-packed-member was resolved: #318 (comment)

@borkmann
Copy link
Member

4.9 -stable request for the kernel bits done here: http://patchwork.ozlabs.org/patch/683818/

@tgraf tgraf added this to the 0.8 release milestone Mar 17, 2017
@aanm aanm self-assigned this Mar 17, 2017
@aanm
Copy link
Member Author

aanm commented Mar 17, 2017

TODO: Provide warning to user.

@tgraf
Copy link
Member

tgraf commented Mar 20, 2017 •
edited by aanm

  • Run clang --version and enforce clang 3.8.x => add to issue with Question tag
  • Check if debug is enabled llc --version and warn because it takes way too long to compile
  • Change deb control file and rpm spec to require llvm 3.8.x
  • Add section to installation docs in requirements
  • Verify if check for libc6-dev is needed => add to issue with Question tag
  • Compile a bunch of prog files in bpf/probes, then grep bpf_features.h for missing features.

@tgraf tgraf assigned borkmann and unassigned borkmann Mar 20, 2017
@borkmann borkmann mentioned this issue Mar 21, 2017
Merged
2 tasks
borkmann added a commit that referenced this issue Mar 21, 2017
@borkmann
bpf: add raw probes for verifier capabilities
9cb50e0 
This work adds a new probe framework for testing the verifier from
raw eBPF byte code. This is needed because some tests cannot be done
through clang, but require specific byte code generation in order to
test the verifier for certain patterns.

We ship two samples, raw_change_tail.t and raw_mark_map_val.t that
also act as templates for future test cases. Each of the *.t files
can hold one or multiple test cases.

Generated header when both tests pass:

  $ ./run_probes.sh . /tmp/
  $ cat /tmp/globals/bpf_features.h
  #ifndef BPF_FEATURES_H_
  #define BPF_FEATURES_H_

  #define HAVE_SKB_CHANGE_TAIL

  #define HAVE_MARK_MAP_VALS

  #endif /* BPF_FEATURES_H_ */

Generated header when not all tests pass, where the verifier output
is dumped in #if 0 / #endif section for further analysis:

  $ ./run_probes.sh . /tmp/
  $ cat /tmp/globals/bpf_features.h
  #ifndef BPF_FEATURES_H_
  #define BPF_FEATURES_H_

  // #define HAVE_SKB_CHANGE_TAIL

  #if 0
  HAVE_SKB_CHANGE_TAIL failed due to load error: Invalid argument
  0: (b7) r2 = 0
  1: (b7) r3 = 0
  2: (85) call 38
  invalid func 38
  #endif

  // #define HAVE_MARK_MAP_VALS

  #if 0
  HAVE_MARK_MAP_VALS failed due to load error: Permission denied
  0: (bf) r2 = r10
  1: (07) r2 += -8
  2: (7a) *(u64 *)(r2 +0) = 0
  3: (18) r1 = 0xd8070cc0
  5: (85) call 1
  6: (bf) r1 = r10
  7: (07) r1 += -152
  8: (7b) *(u64 *)(r1 +0) = r0
  9: (15) if r0 == 0x0 goto pc+2
   R0=map_value(ks=8,vs=8) R1=fp-152 R10=fp fp-152=map_value_or_null
  10: (79) r3 = *(u64 *)(r1 +0)
  11: (7a) *(u64 *)(r3 +0) = 42
  R3 invalid mem access 'map_value_or_null'
  #endif
  #endif /* BPF_FEATURES_H_ */

Newly added test cases just need to add the *.t file to probes, where
run_probes.sh will pick them up automatically. This also removes the
dependency on tc's ability to parse BPF ELF files for test cases.

The test cases can optionally also dump a log notification, which is
later picked up by the cilium daemon and placed into the journal as a
warning on daemon start up, so that the user can see some recommendation
on how to resolve the (potential) issue.

journalctl -fu cilium.service extract:

  [...]
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 007 compileBase > Enabled bpf_jit_enable
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 008 compileBase > Disabled rp_filter on all interfaces!
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 009 compileBase > BPF feature test warnings:
  Mar 21 13:24:06 linux.home cilium-agent[28153]: HAVE_MARK_MAP_VALS: Verifier is too old to detect identical
  registers with map value after bpf_map_lookup_elem(). Some clang versions might generate code that spills
  such registers to stack before a NULL test. Recommendation is to run 4.9+ kernels.
  Mar 21 13:24:07 linux.home cilium-agent[28153]: INFO 00a Infof-fm > Serving cilium at unix:///var/run/cilium/cilium.sock
  Mar 21 13:24:07 linux.home cilium-agent[28153]: INFO 00b Infof-fm > Serving cilium at http://[::]:41557
  [...]

Related: #310

Signed-off-by: Daniel Borkmann <daniel@cilium.io>
tgraf pushed a commit that referenced this issue Mar 21, 2017
@borkmann @tgraf
bpf: add raw probes for verifier capabilities
65922c6 
This work adds a new probe framework for testing the verifier from
raw eBPF byte code. This is needed because some tests cannot be done
through clang, but require specific byte code generation in order to
test the verifier for certain patterns.

We ship two samples, raw_change_tail.t and raw_mark_map_val.t that
also act as templates for future test cases. Each of the *.t files
can hold one or multiple test cases.

Generated header when both tests pass:

  $ ./run_probes.sh . /tmp/
  $ cat /tmp/globals/bpf_features.h
  #ifndef BPF_FEATURES_H_
  #define BPF_FEATURES_H_

  #define HAVE_SKB_CHANGE_TAIL

  #define HAVE_MARK_MAP_VALS

  #endif /* BPF_FEATURES_H_ */

Generated header when not all tests pass, where the verifier output
is dumped in #if 0 / #endif section for further analysis:

  $ ./run_probes.sh . /tmp/
  $ cat /tmp/globals/bpf_features.h
  #ifndef BPF_FEATURES_H_
  #define BPF_FEATURES_H_

  // #define HAVE_SKB_CHANGE_TAIL

  #if 0
  HAVE_SKB_CHANGE_TAIL failed due to load error: Invalid argument
  0: (b7) r2 = 0
  1: (b7) r3 = 0
  2: (85) call 38
  invalid func 38
  #endif

  // #define HAVE_MARK_MAP_VALS

  #if 0
  HAVE_MARK_MAP_VALS failed due to load error: Permission denied
  0: (bf) r2 = r10
  1: (07) r2 += -8
  2: (7a) *(u64 *)(r2 +0) = 0
  3: (18) r1 = 0xd8070cc0
  5: (85) call 1
  6: (bf) r1 = r10
  7: (07) r1 += -152
  8: (7b) *(u64 *)(r1 +0) = r0
  9: (15) if r0 == 0x0 goto pc+2
   R0=map_value(ks=8,vs=8) R1=fp-152 R10=fp fp-152=map_value_or_null
  10: (79) r3 = *(u64 *)(r1 +0)
  11: (7a) *(u64 *)(r3 +0) = 42
  R3 invalid mem access 'map_value_or_null'
  #endif
  #endif /* BPF_FEATURES_H_ */

Newly added test cases just need to add the *.t file to probes, where
run_probes.sh will pick them up automatically. This also removes the
dependency on tc's ability to parse BPF ELF files for test cases.

The test cases can optionally also dump a log notification, which is
later picked up by the cilium daemon and placed into the journal as a
warning on daemon start up, so that the user can see some recommendation
on how to resolve the (potential) issue.

journalctl -fu cilium.service extract:

  [...]
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 007 compileBase > Enabled bpf_jit_enable
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 008 compileBase > Disabled rp_filter on all interfaces!
  Mar 21 13:24:06 linux.home cilium-agent[28153]: WARN 009 compileBase > BPF feature test warnings:
  Mar 21 13:24:06 linux.home cilium-agent[28153]: HAVE_MARK_MAP_VALS: Verifier is too old to detect identical
  registers with map value after bpf_map_lookup_elem(). Some clang versions might generate code that spills
  such registers to stack before a NULL test. Recommendation is to run 4.9+ kernels.
  Mar 21 13:24:07 linux.home cilium-agent[28153]: INFO 00a Infof-fm > Serving cilium at unix:///var/run/cilium/cilium.sock
  Mar 21 13:24:07 linux.home cilium-agent[28153]: INFO 00b Infof-fm > Serving cilium at http://[::]:41557
  [...]

Related: #310

Signed-off-by: Daniel Borkmann <daniel@cilium.io>
@aanm aanm mentioned this issue Mar 22, 2017
Merged
2 tasks
@aanm
Copy link
Member Author

aanm commented Mar 22, 2017

There's one todo left:

  • Add section to installation docs in requirements

@tgraf will you take care of that one?

@aanm aanm reopened this Mar 22, 2017
@tgraf
Copy link
Member

tgraf commented Mar 22, 2017

@aanm Yes. I'll reassign this to me then.

@tgraf tgraf assigned tgraf and unassigned borkmann and aanm Mar 22, 2017
@tgraf tgraf mentioned this issue Mar 25, 2017
Merged
tgraf added a commit that referenced this issue Mar 25, 2017
@tgraf
README: Update prerequisites
c31fed3 
Fixes: #310

Signed-off-by: Thomas Graf <thomas@cilium.io>
@aanm aanm closed this as completed in #453 Mar 25, 2017
aanm pushed a commit that referenced this issue Mar 25, 2017
@tgraf @aanm
README: Update prerequisites
8671d7f 
Fixes: #310

Signed-off-by: Thomas Graf <thomas@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tgraf tgraf

Labels
kind/bug This is a bug in the Cilium logic.
Projects
None yet
Milestone
0.8 release
Development

Successfully merging a pull request may close this issue.

daemon: checking minimal requirements ASAP cilium/cilium
3 participants
@borkmann @tgraf @aanm