bmahler commented on code in PR #501: URL: https://github.com/apache/mesos/pull/501#discussion_r1518049989
########## src/linux/ebpf.hpp: ########## @@ -14,7 +14,128 @@ // See the License for the specific language governing permissions and // limitations under the License. +// TODO(dleamy): Look into using libbpf: https://github.com/libbpf/libbpf +// to simplify and or replace the low-level BPF operations. + #ifndef __EBPF_HPP__ #define __EBPF_HPP__ +#include <linux/bpf.h> + +#include <vector> + +#include "stout/try.hpp" + +namespace ebpf { + +// eBPF program. +class Program +{ +public: + explicit Program(bpf_prog_type type); + + // Append instructions to the end of the eBPF program. + void append(std::vector<bpf_insn>&& instructions); + + // Create a eBPF attribute for the program that can be loaded + // into the kernel. + bpf_attr createAttribute() const; + +protected: + // Type of eBPF program. + const bpf_prog_type type; + + // Instructions of the eBPF program. + std::vector<bpf_insn> program; +}; + + +// Loads the provided eBPF program into the kernel and returns the file +// descriptor of loaded program. +Try<int> load(const Program&& program); Review Comment: remove const here if you want to take an rvalue reference ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); Review Comment: let's remove this macro, it isn't adding much value at this point ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} Review Comment: let's point to the lwn article I sent you on why glibc doesn't expose a function for this syscall, to explain why we need to make the direct syscall here ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} Review Comment: this is still a dangerous function because the caller *must* keep Program alive longer than the bpf_attr returned by this function, and that's not obvious (e.g. this is called create and returns a value, rather than, say, a const ref) and easy to get wrong and cause a use-after-free bug we could instead just expose the instructions and the license, and have `load(const Program& program)` build the bpf_attr struct needed for loading, this also avoids the need to do any moving since Program clearly outlives load ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. Review Comment: this is clear, no need for this comment ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} + + +Try<int> load(const Program&& program) +{ + bpf_attr attribute = program.createAttribute(); + string logs = ""; + + // The BPF system call is repeated based on whether it succeeds and the + // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN' + // we retry with the same parameters. If it fails with 'EACCES' - a + // verification error - when we retry once with an additional buffer to + // capture the verifier error logs. + // + // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is + // returned multiple times. + int attempts = 0; + bool retry = false; + bool retriedWithLogs = false; + + Try<int> fd = 0; // '0' because Try<int> has no default constructor. + do { Review Comment: once EAGAIN is moved into the wrapper, we can avoid a loop and the three loop related variables ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} + + +Try<int> load(const Program&& program) +{ + bpf_attr attribute = program.createAttribute(); + string logs = ""; + + // The BPF system call is repeated based on whether it succeeds and the + // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN' + // we retry with the same parameters. If it fails with 'EACCES' - a + // verification error - when we retry once with an additional buffer to + // capture the verifier error logs. + // + // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is + // returned multiple times. + int attempts = 0; + bool retry = false; + bool retriedWithLogs = false; + + Try<int> fd = 0; // '0' because Try<int> has no default constructor. + do { + fd = bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute)); + ++attempts; + + if (fd.isSome()) { + break; + } + + ErrnoError error; + switch (error.code) { + case EAGAIN: { + // Retry with the same parameters. + retry = true; + break; + } + case EACCES: { + // Retry and capture verifier error logs, if we haven't already. + if (retriedWithLogs) { + retry = false; + break; + } + + retry = true; + vector<char> verifier_log_buffer(8196); + attribute.log_level = 1; + attribute.log_buf = + reinterpret_cast<uint64_t>(verifier_log_buffer.data()); + attribute.log_size = verifier_log_buffer.size(); + fd = bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute)); Review Comment: perhaps add a CHECK here which requires using `Try<int, ErrnoError>` as the bpf return type: ``` CHECK_ERROR(fd); CHECK_EQ(EACCESS, fd.error().code); ``` ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); Review Comment: per offline discussion, it would be good to deal with EAGAIN here, and reference where libbpf uses a default of 5 EAGAIN retries to show why we chose this number of retries: https://github.com/libbpf/libbpf/blob/master/src/bpf.h#L71-L75 ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,99 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <errno.h> +#include <linux/bpf.h> +#include <sys/syscall.h> Review Comment: yeah, I think it's actually supposed to be glibc that exposes it per the lwn article, so we can reference that article below where we make a syscall ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); Review Comment: ``` return ErrnoError(); ``` note that the caller is aware that this is a bpf() call and they will compose that into their error message, we only need to include the reason for the bpf call failure ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); Review Comment: AFAICT this is safe because C++ guarantees that string literals have static storage and are alive for the lifetime of the program, but it would be more obviously safe here to just have a const LICENSE string stored statically in this .cpp file. ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} + + +Try<int> load(const Program&& program) +{ + bpf_attr attribute = program.createAttribute(); + string logs = ""; + + // The BPF system call is repeated based on whether it succeeds and the + // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN' + // we retry with the same parameters. If it fails with 'EACCES' - a + // verification error - when we retry once with an additional buffer to + // capture the verifier error logs. + // + // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is + // returned multiple times. + int attempts = 0; + bool retry = false; + bool retriedWithLogs = false; + + Try<int> fd = 0; // '0' because Try<int> has no default constructor. Review Comment: once the loop is removed, this = 0 can be avoided by assigning it on the first of the two bpf calls ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} + + +Try<int> load(const Program&& program) +{ + bpf_attr attribute = program.createAttribute(); + string logs = ""; Review Comment: we shouldn't need this extra string and can use the verifier log buffer directly ########## src/linux/ebpf.cpp: ########## @@ -14,4 +14,126 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "linux/ebpf.hpp" \ No newline at end of file +#include "linux/ebpf.hpp" + +#include <linux/bpf.h> +#include <sys/syscall.h> + +#include <string> +#include <vector> + +#include "stout/error.hpp" +#include "stout/try.hpp" + +using std::string; +using std::vector; + +namespace ebpf { + +#define syscall_bpf(cmd, attr, size) (int) syscall(__NR_bpf, cmd, attr, size); + +// Wrapper around the BPF syscall. +Try<int> bpf(int cmd, bpf_attr* attr, size_t size) +{ + int result = syscall_bpf(cmd, attr, size); + if (result == -1) { + ErrnoError error; + return Error("BPF syscall failed with error '" + error.message + "'"); + } + return result; +} + + +Program::Program(bpf_prog_type _type) : type(_type) {} + + +void Program::append(vector<bpf_insn>&& instructions) +{ + program.insert( + program.end(), + std::make_move_iterator(instructions.begin()), + std::make_move_iterator(instructions.end())); +} + + +bpf_attr Program::createAttribute() const +{ + bpf_attr attribute; + std::memset(&attribute, 0, sizeof(attribute)); + + attribute.prog_type = type; + attribute.insn_cnt = program.size(); + attribute.insns = reinterpret_cast<uint64_t>(program.data()); + attribute.license = reinterpret_cast<uint64_t>("Apache 2.0"); + + return attribute; +} + + +Try<int> load(const Program&& program) +{ + bpf_attr attribute = program.createAttribute(); + string logs = ""; + + // The BPF system call is repeated based on whether it succeeds and the + // return value. If it succeeds, we don't retry. It it fails with 'EAGAIN' + // we retry with the same parameters. If it fails with 'EACCES' - a + // verification error - when we retry once with an additional buffer to + // capture the verifier error logs. + // + // We attempt a maximum of 3 times, to avoid an infinite loop if 'EAGAIN' is + // returned multiple times. + int attempts = 0; + bool retry = false; + bool retriedWithLogs = false; + + Try<int> fd = 0; // '0' because Try<int> has no default constructor. + do { + fd = bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute)); + ++attempts; + + if (fd.isSome()) { + break; + } + + ErrnoError error; + switch (error.code) { + case EAGAIN: { + // Retry with the same parameters. + retry = true; + break; + } + case EACCES: { + // Retry and capture verifier error logs, if we haven't already. + if (retriedWithLogs) { + retry = false; + break; + } + + retry = true; + vector<char> verifier_log_buffer(8196); + attribute.log_level = 1; + attribute.log_buf = + reinterpret_cast<uint64_t>(verifier_log_buffer.data()); + attribute.log_size = verifier_log_buffer.size(); + fd = bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute)); + logs = ": " + string(verifier_log_buffer.data()); + retry = true; + break; + } + default: { + logs = ": Unexpcted error from BPF syscall: " + error.message; + retry = false; + break; Review Comment: we won't need this anymore, we can just do a: ``` return Error("bpf syscall failed: " + fd.error()); ... return Error("bpf verifier failed: " + &verifier_log_buffer[0]); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@mesos.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
