bmahler commented on code in PR #501:
URL: https://github.com/apache/mesos/pull/501#discussion_r1516633100
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
Review Comment:
unused?
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
Review Comment:
you'll generally want to prefer strerror_r to strerror, but you almost never
want to be calling this directly, you can simply use `return ErrnoError()` here.
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
Review Comment:
looks like we can use the bpf call from linux/bpf.h:
https://man7.org/linux/man-pages/man2/bpf.2.html
then we don't need this extra wrapper
##########
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>
+
+#include <cstdlib>
Review Comment:
do you use anything from here? do you want <cstdint> instead?
##########
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>
+
+#include <cstdlib>
+#include <cstring>
Review Comment:
shouldn't need this once strerror is removed?
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
Review Comment:
hm.. you should just be able to use static casts below instead of adding
this:
```
attribute.insns = static_cast<uint64_t>(program.data());
attribute.license = static_cast<uint64_t>("Apache 2.0");
```
this seems to be one area where libbpf provides a nicer api that does this
casting etc for us:
```
int bpf_prog_load (enum bpf_prog_type prog_type, const char *prog_name,
const char *license, const struct bpf_insn *insns, size_t insn_cnt, struct
bpf_prog_load_opts *opts)
```
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
Review Comment:
similarly to my comment below, we don't need to use `enum` here
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
Review Comment:
in the cpp you can avoid std:: qualifications with using statements above,
you actually already have one for vector
##########
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:
you won't need this if you remove the direct syscall
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
Review Comment:
ditto here, don't know if you need the union here?
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(&attribute, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
+ attribute.license = internal::pointer_to_int("Apache 2.0");
+
+ return Program{.attribute = attribute};
+}
+
+
+Try<int> load(const Program& program, bool logErrors)
+{
+ union bpf_attr attribute = program.attribute;
+ Try<int> fd = 0; // placeholder because Try<int>() DNE
+ string logs = "";
+
+ if (logErrors) {
+ const int LOG_SIZE = 8196;
+ const int LOG_LEVEL = 1;
+
+ char* log_buffer = (char*)malloc(LOG_SIZE);
+ attribute.log_level = LOG_LEVEL;
+ attribute.log_buf = internal::pointer_to_int(log_buffer);
+ attribute.log_size = LOG_SIZE;
+ fd = internal::syscall_bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute));
+ logs = ": " + string(log_buffer);
+ } else {
+ fd = internal::syscall_bpf(BPF_PROG_LOAD, &attribute, sizeof(attribute));
+ }
+
+ if (fd.isError()) {
+ return Error("BPF syscall failed" + logs + ": " + fd.error());
+ }
Review Comment:
It looks like the log_level is only for the verifier? The docs suggest that
we should not include logging initially (for overhead reduction), then call
again with logging when appropriate, we would then include this error
information directly in the Error.
```
EACCES For BPF_PROG_LOAD, even though all program instructions
are valid, the program has been rejected because it was
deemed unsafe. This may be because it may have accessed a
disallowed memory region or an uninitialized
stack/register or because the function constraints don't
match the actual types or because there was a misaligned
memory access. In this case, it is recommended to call
bpf() again with log_level = 1 and examine log_buf for the
specific reason provided by the verifier.
```
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(&attribute, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
+ attribute.license = internal::pointer_to_int("Apache 2.0");
+
+ return Program{.attribute = attribute};
+}
+
+
+Try<int> load(const Program& program, bool logErrors)
+{
+ union bpf_attr attribute = program.attribute;
+ Try<int> fd = 0; // placeholder because Try<int>() DNE
+ string logs = "";
+
+ if (logErrors) {
+ const int LOG_SIZE = 8196;
Review Comment:
if we use a big size like this, consider using a vector<char> here as the
buffer so it's on the heap and managed, and zeroed for us
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(&attribute, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
+ attribute.license = internal::pointer_to_int("Apache 2.0");
+
+ return Program{.attribute = attribute};
+}
+
+
+Try<int> load(const Program& program, bool logErrors)
+{
+ union bpf_attr attribute = program.attribute;
+ Try<int> fd = 0; // placeholder because Try<int>() DNE
+ string logs = "";
+
+ if (logErrors) {
+ const int LOG_SIZE = 8196;
+ const int LOG_LEVEL = 1;
+
+ char* log_buffer = (char*)malloc(LOG_SIZE);
Review Comment:
this is a memory leak! we could stack (or unique_ptr + new) allocate it
inside the if EACCESS if condition:
```cpp
char verifier_log_buffer[VERIFIER_LOG_SIZE];
// Ensure the log is null terminated if the verifier doesn't write anything.
verifier_log_buffer[0] = '\0';
attribute.log_buf = static_cast<uint64_t>(verifier_log_buffer);
// We don't bother looking for ENOSPC because we use a very large buffer
size.
XXX
// Maybe check that it's EACCESS again..?
ErrnoError error;
CHECK_EQ(EACCESS, error.errno) << "Expected verifier error again, got: " <<
error;
return Error("bpf verifier rejected the program: " +
string(verifier_log_buffer))
```
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
+};
+
+
+// Builder for eBPF programs.
+class ProgramBuilder
+{
+public:
+ explicit ProgramBuilder(enum bpf_prog_type type);
+
+ // Append instructions to the end of the eBPF program.
+ void append(std::vector<struct bpf_insn> instructions);
+
+ // Number of instructions in the eBPF program.
+ size_t instructions() const;
+
+ // Build a loadable eBPF program.
+ Program build();
+
+protected:
+ // Type of eBPF program.
+ const enum bpf_prog_type type;
+
+ // Instructions of the eBPF program.
+ std::vector<struct bpf_insn> program;
+};
+
+
+// Loads the provided eBPF program into the kernel and returns the file
+// descriptor of loaded program.
+// If `logErrors` is true, logs will be returned in the error message if the
+// load fails.
+Try<int> load(const Program& program, bool logErrors = false);
Review Comment:
Taking logErrors isn't idiomatic for our code, you would just return the
errors within the Try typically, any reason this is special? It also reads as
if this function with `LOG(ERROR)` when this is true, based on the variable
name.
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
Review Comment:
unused?
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
Review Comment:
unused? note that os has subheaders if you do need something from here, that
let you avoid pulling in the whole os header files which are quite large
##########
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>
Review Comment:
you won't need this if you use ErrnoError
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
+};
+
+
+// Builder for eBPF programs.
+class ProgramBuilder
+{
+public:
+ explicit ProgramBuilder(enum bpf_prog_type type);
+
+ // Append instructions to the end of the eBPF program.
+ void append(std::vector<struct bpf_insn> instructions);
+
+ // Number of instructions in the eBPF program.
+ size_t instructions() const;
Review Comment:
do we need this..? seems like an odd thing to expose, if it's absolutely
needed, we could call it size() instead which would be more idiomatic
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
Review Comment:
don't think we need the `union` here? (similar to my other comments)
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
Review Comment:
can you instead take an rvalue reference here and move the instructions in
appropriately?
also, note that you don't need to use `struct` here since we're in C++, and
we don't need an implicit forward declaration here (I think that's the only
need for it in C++)
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
Review Comment:
perhaps a TODO at the top of this header that we should investigate using
libbpf (much like we use libnl) and whether that simplifies our code here
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
+};
+
+
+// Builder for eBPF programs.
+class ProgramBuilder
+{
+public:
+ explicit ProgramBuilder(enum bpf_prog_type type);
Review Comment:
ditto here with enum
##########
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>
+
+#include <cstdlib>
+#include <cstring>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "stout/foreach.hpp"
+#include "stout/os.hpp"
+
+using std::ostream;
+using std::string;
+using std::vector;
+
+namespace ebpf {
+
+namespace internal {
+
+// Wrapper around the `bpf` syscall.
+Try<int> syscall_bpf(int cmd, union bpf_attr* attr, size_t size)
+{
+ int result = syscall(__NR_bpf, cmd, attr, size);
+ if (result == -1) {
+ return Error(
+ "BFP syscall failed with error '" + string(std::strerror(errno)) + "'");
+ }
+ return result;
+}
+
+
+uint64_t pointer_to_int(const void* pointer)
+{
+ return (uint64_t)(unsigned long)pointer;
+}
+
+} // namespace internal {
+
+ProgramBuilder::ProgramBuilder(enum bpf_prog_type _type) : type(_type) {}
+
+
+void ProgramBuilder::append(std::vector<struct bpf_insn> instructions)
+{
+ program.insert(program.end(), instructions.begin(), instructions.end());
+}
+
+
+size_t ProgramBuilder::instructions() const { return program.size(); }
+
+
+Program ProgramBuilder::build()
+{
+ union bpf_attr attribute;
+ std::memset(&attribute, 0, sizeof(attribute));
+
+ attribute.prog_type = type;
+ attribute.insn_cnt = instructions();
+ attribute.insns = internal::pointer_to_int(program.data());
Review Comment:
ah maybe this is where you wanted `instructions()` to get the size of
program? it would be more clear here to keep the program.data() and
program.size() connected:
```
attribute.insn_cnt = program.size();
attribute.insns = static_cast<uint64_t>(program.data());
```
##########
src/linux/ebpf.hpp:
##########
@@ -17,4 +17,136 @@
#ifndef __EBPF_HPP__
#define __EBPF_HPP__
+#include <linux/bpf.h>
+
+#include <cstddef>
+#include <vector>
+
+#include "stout/nothing.hpp"
+#include "stout/try.hpp"
+
+namespace ebpf {
+
+// Loadable eBPF program. Constructed using an ebpf::ProgramBuilder.
+struct Program
+{
+ // eBPF attribute containing an eBPF program.
+ union bpf_attr attribute;
+};
+
+
+// Builder for eBPF programs.
+class ProgramBuilder
+{
+public:
+ explicit ProgramBuilder(enum bpf_prog_type type);
+
+ // Append instructions to the end of the eBPF program.
+ void append(std::vector<struct bpf_insn> instructions);
+
+ // Number of instructions in the eBPF program.
+ size_t instructions() const;
+
+ // Build a loadable eBPF program.
+ Program build();
Review Comment:
This API is not memory safe since the caller needs to keep the
ProgramBuilder around longer than the returned Program from this function.
It might be worth just having `Program` which can have instructions added,
as well as can be loaded. There doesn't really seem to be a need to separate
the builder from the program here, and it's causing memory safety issues in the
API.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
