Skip to content

n-acd: decide if ebpf is used at runtime #9

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

n-acd: decide if ebpf is used at runtime #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,6 @@ meson test
ninja install
```

The following configuration options are available:

* `ebpf`: This boolean controls whether `ebpf` features are used to improve
the package filtering performance. If disabled, classic bpf will be
used. This feature requires a rather recent kernel (>=3.19).
Default is: true

### Repository:

- **web**: <https://github.com/nettools/n-acd>
Expand Down
2 changes: 0 additions & 2 deletions meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,4 @@ dep_crbtree = sub_crbtree.get_variable('libcrbtree_dep')
dep_csiphash = sub_csiphash.get_variable('libcsiphash_dep')
dep_cstdaux = sub_cstdaux.get_variable('libcstdaux_dep')

use_ebpf = get_option('ebpf')

subdir('src')
1 change: 0 additions & 1 deletion meson_options.txt
View file
Open in desktop

This file was deleted.

17 changes: 3 additions & 14 deletions src/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,11 @@ libnacd_deps = [

libnacd_sources = [
'n-acd.c',
'n-acd-bpf.c',
'n-acd-probe.c',
'util/timer.c',
]

if use_ebpf
libnacd_sources += [
'n-acd-bpf.c',
]
else
libnacd_sources += [
'n-acd-bpf-fallback.c',
]
endif

libnacd_private = static_library(
'nacd-private',
libnacd_sources,
Expand Down Expand Up @@ -77,10 +68,8 @@ endif
test_api = executable('test-api', ['test-api.c'], link_with: libnacd_shared)
test('API Symbol Visibility', test_api)

if use_ebpf
test_bpf = executable('test-bpf', ['test-bpf.c'], dependencies: libnacd_dep)
test('eBPF socket filtering', test_bpf)
endif
test_bpf = executable('test-bpf', ['test-bpf.c'], dependencies: libnacd_dep)
test('eBPF socket filtering', test_bpf)

test_loopback = executable('test-loopback', ['test-loopback.c'], dependencies: libnacd_dep)
test('Echo Suppression via Loopback', test_loopback)
Expand Down
30 changes: 0 additions & 30 deletions src/n-acd-bpf-fallback.c
View file
Open in desktop

This file was deleted.

6 changes: 2 additions & 4 deletions src/n-acd-probe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,6 @@ static int n_acd_probe_link(NAcdProbe *probe) {
* entry.
*/
r = n_acd_ensure_bpf_map_space(probe->acd);
if (r)
return r;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this error-handler there, otherwise we will suppress real errors from the bpf-helpers.


/*
* Link entry into context, indexed by its IP. Note that we allow
Expand Down Expand Up @@ -238,7 +236,7 @@ static int n_acd_probe_link(NAcdProbe *probe) {
/*
* Add the ip address to the map, if it is not already there.
*/
if (n_acd_probe_is_unique(probe)) {
if (probe->acd->fd_bpf_map >= 0 && n_acd_probe_is_unique(probe)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would rather prefer for n_acd_bpf_map_add() to return success early if the passed map is negative. The reason is that we run in a process we do not control, so technically EUID can change during runtime and the bpf-calls might suddenly succeed. If our state always reflects the state as if the BPF map was there, then any call that suddenly succeeds in creating the BPF-map will correctly add all the IPs and have n_bpf_map reflect the real required state.

r = n_acd_bpf_map_add(probe->acd->fd_bpf_map, &probe->ip);
if (r) {
/*
Expand All @@ -261,7 +259,7 @@ static void n_acd_probe_unlink(NAcdProbe *probe) {
* If this is the only probe for a given IP, remove the IP from the
* kernel BPF map.
*/
if (n_acd_probe_is_unique(probe)) {
if (probe->acd->fd_bpf_map >= 0 && n_acd_probe_is_unique(probe)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above. I would rather make bpf_map_remove() bail out early with success if the passed map-fd is negative.

r = n_acd_bpf_map_remove(probe->acd->fd_bpf_map, &probe->ip);
c_assert(r >= 0);
--probe->acd->n_bpf_map;
Expand Down
12 changes: 6 additions & 6 deletions src/n-acd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ int n_acd_ensure_bpf_map_space(NAcd *acd) {

r = n_acd_bpf_compile(&fd_prog, fd_map, (struct ether_addr*) acd->mac);
if (r)
return r;
fd_prog = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will suppress any real error from n_acd_bpf_compile(). Why not add N_ACD_E_DENIED after N_ACD_E_DROPPED in n-acd-private.h and make n_acd_bpf_compile() return that in the exact circumstances where it matters? Then make all callers of it handle that error gracefully, just like you did here.


if (fd_prog >= 0) {
r = setsockopt(acd->fd_socket, SOL_SOCKET, SO_ATTACH_BPF, &fd_prog, sizeof(fd_prog));
Expand Down Expand Up @@ -360,12 +360,12 @@ _c_public_ int n_acd_new(NAcd **acdp, NAcdConfig *config) {
acd->max_bpf_map = 8;

r = n_acd_bpf_map_create(&acd->fd_bpf_map, acd->max_bpf_map);
if (r)
return r;

r = n_acd_bpf_compile(&fd_bpf_prog, acd->fd_bpf_map, (struct ether_addr*) acd->mac);
if (r)
return r;
if (acd->fd_bpf_map >= 0) {
r = n_acd_bpf_compile(&fd_bpf_prog, acd->fd_bpf_map, (struct ether_addr*) acd->mac);
if (r)
fd_bpf_prog = -1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I think this should handle a possible N_ACD_E_DENIED. So something like this:

fd_bpf_prog = -1;
r = n_acd_bpf_map_create(...);
if (!r)
    r = n_acd_bpf_compile(...);
if (r && r != N_ACD_E_DENIED)
    return r;


r = n_acd_socket_new(&acd->fd_socket, fd_bpf_prog, config);
if (r)
Expand Down
19 changes: 13 additions & 6 deletions src/test-bpf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@ static void test_map(void) {
struct in_addr addr = { 1 };

r = n_acd_bpf_map_create(&mapfd, 8);
c_assert(r >= 0);
c_assert(mapfd >= 0);

if (r == -EPERM)
return;

r = n_acd_bpf_map_remove(mapfd, &addr);
c_assert(r == -ENOENT);
Expand Down Expand Up @@ -103,7 +104,13 @@ static void test_filter(void) {
int r, mapfd = -1, progfd = -1, pair[2];

r = n_acd_bpf_map_create(&mapfd, 1);

if (r == -EPERM)
return;

c_assert(r >= 0);
c_assert(mapfd >= 0);


r = n_acd_bpf_compile(&progfd, mapfd, &mac1);
c_assert(r >= 0);
Expand All @@ -113,7 +120,7 @@ static void test_filter(void) {
c_assert(r >= 0);

r = setsockopt(pair[1], SOL_SOCKET, SO_ATTACH_BPF, &progfd,
sizeof(progfd));
sizeof(progfd));
c_assert(r >= 0);

r = n_acd_bpf_map_add(mapfd, &ip1);
Expand Down Expand Up @@ -190,9 +197,9 @@ static void test_filter(void) {
c_assert(errno == EAGAIN);

/*
* Send one packet before and one packet after modifying the map,
* verify that the modification applies at the time of send(), not recv().
*/
* Send one packet before and one packet after modifying the map,
* verify that the modification applies at the time of send(), not recv().
*/
*packet = (struct ether_arp)ETHER_ARP_PACKET_INIT(ARPOP_REQUEST, &mac2, &ip1, &ip2);
r = send(pair[0], buf, sizeof(struct ether_arp), 0);
c_assert(r == sizeof(struct ether_arp));
Expand Down