bg/n-acd-update: update n-acd code from upstream #195

bengal · 2018-09-07T19:53:07Z

Update the n-acd code to the upstream version, after the release of version 1 with stable API and (optional) support for eBPF filtering.

thom311 · 2018-09-08T07:38:32Z

configure.ac

+if test "$with_ebpf" != "yes" -a "$with_ebpf" != "no"; then
+	AC_MSG_ERROR(--with-ebpf must be one of [yes, no])
+fi
+AM_CONDITIONAL(WITH_EBPF, test "${with_ebpf}" = "yes")


shouldn't we autodetect support, and only provide the --with-ebpf option if somebody needs to change the autodetection?

ebpf is supported since kernel 3.19, which is more than 3 years old.
I think defaulting to yes is fine, while still allowing to disable it explicitly.

thom311 · 2018-09-08T07:44:29Z

src/devices/nm-acd-manager.c

@@ -472,7 +496,15 @@ dispose (GObject *object)
 	NMAcdManager *self = NM_ACD_MANAGER (object);
 	NMAcdManagerPrivate *priv = NM_ACD_MANAGER_GET_PRIVATE (self);

+	g_clear_pointer (&priv->hwaddr, g_free);


nm_clear_g_free()?

thom311 · 2018-09-08T07:44:44Z

src/devices/nm-acd-manager.c

+	if (priv->acd) {
+		n_acd_unref (priv->acd);
+		priv->acd = NULL;
+	}


nm_clear_pointer(&priv->acd, n_acd_unref)?

thom311 · 2018-09-08T07:46:52Z

src/devices/nm-acd-manager.c

 	g_return_val_if_fail (hwaddr, NULL);
 	g_return_val_if_fail (hwaddr_len == ETH_ALEN, NULL);

 	self = g_object_new (NM_TYPE_ACD_MANAGER, NULL);
 	priv = NM_ACD_MANAGER_GET_PRIVATE (self);
 	priv->ifindex = ifindex;
-	memcpy (priv->hwaddr, hwaddr, ETH_ALEN);
+	priv->hwaddr = g_memdup (hwaddr, hwaddr_len);


nm_memdup(). There is little difference in practice, but I think we should restrain ourself from API that is error prone.

thom311 · 2018-09-08T07:47:59Z

src/devices/nm-acd-manager.c

@@ -54,10 +51,14 @@ static guint signals[LAST_SIGNAL] = { 0 };

 typedef struct {
 	int            ifindex;
-	guint8         hwaddr[ETH_ALEN];
+	guint8         *hwaddr;
+	size_t         hwaddr_len;


guint? Because at other places, the length is limited to guint type. Or, adjust the other places.

thom311 · 2018-09-08T07:48:29Z

src/devices/nm-acd-manager.c

 {
 	NMAcdManager *self;
 	NMAcdManagerPrivate *priv;

+	g_return_val_if_fail (ifindex > 0, NULL);
 	g_return_val_if_fail (hwaddr, NULL);
 	g_return_val_if_fail (hwaddr_len == ETH_ALEN, NULL);


if you assert that the hwaddr_len is ETH_ALEN, no priv->hwaddr_len is needed.

thom311 · 2018-09-08T07:52:55Z

src/devices/nm-acd-manager.c


-	r = n_acd_new (&info->acd);
+	r = n_acd_probe_config_new (&probe_config);
 	if (r) {


at various places, isn't the convention to check for error with if (r < 0)?

ACD functions return 0 on success. On error they return a positive (N_ACD_E_PREEMPTED, N_ACD_E_INVALID_ARGUMENT) or negative (-errno) error code, so the code looks correct.

ok. That was not obvious to me. Maybe if (r != N_ACD_E_SUCCESS)?

Or, fine as is with if (r). If you prefer that.

thom311 · 2018-09-08T07:54:16Z

src/devices/nm-acd-manager.c

@@ -117,21 +118,15 @@ static const char *
 _acd_error_to_string (int error)


the name does not seem optimal. Positive numbers are not really errors, are they?

Yes, N_ACD_E_PREEMPTED and N_ACD_E_INVALID_ARGUMENT are errors.

_N_ACD_E_SUCCESS of course is not, but I think _acd_error_to_string() is fine as a name for a function that translates error codes to string.

bengal · 2018-09-13T14:49:30Z

Fixed the issues above and repushed.

thom311 · 2018-09-17T14:58:28Z

src/devices/nm-acd-manager.c

 	case N_ACD_E_PREEMPTED:
 		return "preempted";
 	case N_ACD_E_INVALID_ARGUMENT:
 		return "invalid argument";
-	case N_ACD_E_BUSY:
-		return "busy";
 	}
 	return NULL;


maybe g_return_val_if_reached (NULL). In this case, n-acd is a static library that is compiled in. This isn't (really) an external component, and an unknown error code is a bug that we should assert against. And then drop the acd_error_to_string() macro.

thom311

lgtm

git-subtree-dir: shared/c-rbtree git-subtree-split: bf627e0c32241915108f66ad9738444e4d045b45

…rbtree' Imported c-rbtree code with command: git subtree add --prefix shared/c-rbtree git@github.com:c-util/c-rbtree.git bf627e0c32241915108f66ad9738444e4d045b45 --squash To update the library use: git subtree pull --prefix shared/c-rbtree git@github.com:c-util/c-rbtree.git master --squash

a40949267 build: add CI run without ebpf 044db2056 n-acd: drop redundant headers 6a391cd83 n-acd: fix build without eBPF bb194cf09 n-acd/config: make transport mandatory ec2865743 build: drop unused c-sundry 721d9d84f n-acd: inline c_container_of() 1a7ee317c util/timer: fix coding-style 6c96f926b util/timer: fall back to CLOCK_MONOTONIC if necessary 4ea3165fc n-acd: only use CLOCK_BOOTTIME if really necessary c1b853c6c util/timer: cleanup headers b1d6ad272 n-acd: add destructors that return void 185be55b6 test-bpf: skip test in case of unsufficient privs 84a40e8fa build: add NEWS file bf11443ff build: mention mailinglist in readme e2797984a test-bpf: drop bpf-filter.h 668ed3c82 subprojects: pull in updates dd8cab3f0 test-veth: reduce parallel execution to 9 68b09ba2b build: update AUTHORS 3f77e3e88 test: make function headers valid C 5275a5120 test: get rid of spurious tab 037df412c n-acd: make struct initializers valid C 346ec0c67 build: upgrade CI 38682a36d n-acd: fix signed vs unsigned comparison 5e7578b33 bpf: properly zero out trailing bpf_attr space ee1e432ae probe: fix coding-style a143540f9 build: use lower-case build options 835533e7d build: minor style fixes 2bd6d1d29 build: get rid of tabs b14979934 eBPF: make compile-time optional 6f13c27ee n-acd: filter out invalid packets 4e6a169a0 build: sync with c-util repositories 6c4a9117b build: document eBPF kernel requirement 3ef08394d n-acd: don't remember dropped defense attempts 4dff8771f n-acd: fix coding-style b11fb9706 n-acd/config: default to the RFC-specified timeout d885bb3b7 n-acd/event: don't expose the type of operation that caused a conflict e2f87e047 TODO: drop remaining items f06993856 test/veth: reduce the number of probed addresses 8b4f7ed64 test/veth: bump the timeout a bit 14e4606f6 n-acd/probe: don't cap the jitter at 4s a0247b86f test/veth: fix stackvariable corruption a64ac8389 n-acd/probe: update comments aa9c25bc1 n-acd/handle_timeout: update comments b6c2df3a9 timer: rename timer_pop() to timer_pop_timeout() 47c657a8d test: fix handling of child addresses 27168ba9e timer: move timer_read() from n-acd.c to util/timer.c 21a1e37aa timer: require timer to be explicitly rearmed ee1080820 bpf/map: make key/value sizes self-documenting fd444353e test/veth: rework test ba2bc433c test: rework child_ip() helper 07881b8da test: silence a warning 38da00b0a test/bpf: make tests for map modifications more comprehensive 6a2ffd23a test/timerfd: for documentation purposes verify the kernel API 01a9cf54b probe: move from ms to ns internally 4fe438dd9 n-acd: move to use the Timer utility library e098cfc79 util: add a timer utility helper 8ea196e5b subprojects: pull in c-sundry 0c0b3c29f acd/probe: do not subscribe to packets in FAILED state 9c922ea3d acd/probe: introduce probe_{un,}link() helpers 024a830e6 acd/probe: use unschedule() helper in free() b098a3bcc tests/veth: minor fixes to the test fe3d9578a acd/packet: consider unexpected packets a fatal error 34d7656d7 acd: stop state-machine after USED or CONFLICT events 7d9e5ec6b acd: don't declare iovec entries inline 7afd8d8a3 tests: add veth test 26a737b42 tests/veth: add helper for adding IP addresses to child device e73a37a11 probe: store a userdata pointer in the probe object 327e82625 test: introduce loopback helper 0682b15f8 acd: reduce default map size afead881f tests: reinstate loopback test 4527d2f71 BPF: move and document the eBPF helpers 88bacc022 socket filter: move to the new eBPF helpers 245104d5c tests: skip tests if lacking permissions 195d9ff5a n-acd: rework API to support many probes on a context ab440eb99 eBPF: never return packets that userspace should unconditionally drop ac933f412 eBPF: add eBPF helper functions git-subtree-dir: shared/n-acd git-subtree-split: a40949267923c45cb232fa4c1d60eafacee4b36e

…-update git subtree pull --prefix shared/n-acd git@github.com:nettools/n-acd.git master --squash

If configure.ac automatically adds compiler flags to CFLAGS, it becomes hard to override one of them for a specific target because CFLAGS is added last. It is better to use AM_CFLAGS. See [1]. [1] https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html

Adapt the nm-acd-manager.c code to the new API and also tweak build options to the new project structure.

Add a configure option to disable eBPF support in n-acd. Note that, even if eBPF is not supported, n-acd requires a kernel > 3.19, which means that the setsockopt(..., SO_ATTACH_BPF) option must be defined. To allow building on older kernels without modifying the n-acd code, we inject the SO_ATTACH_BPF value as a preprocessor define in the compiler the command line.

thom311 reviewed Sep 8, 2018

View reviewed changes

thom311 assigned bengal Sep 11, 2018

lkundrak force-pushed the bg/n-acd-update branch 2 times, most recently from 9514e09 to 34903a6 Compare September 13, 2018 14:56

thom311 reviewed Sep 17, 2018

View reviewed changes

thom311 approved these changes Sep 17, 2018

View reviewed changes

bengal added 8 commits September 18, 2018 15:08

Squashed 'shared/c-rbtree/' content from commit bf627e0c3

4f4e966

git-subtree-dir: shared/c-rbtree git-subtree-split: bf627e0c32241915108f66ad9738444e4d045b45

Merge commit '1361ede099f51cc29b478ebee6a736219ad74b97' into bg/n-acd…

d0c32a1

…-update git subtree pull --prefix shared/n-acd git@github.com:nettools/n-acd.git master --squash

build: compile the c-rbtree library

88072c6

acd: adapt NM code and build options

d9a4b59

Adapt the nm-acd-manager.c code to the new API and also tweak build options to the new project structure.

lkundrak force-pushed the bg/n-acd-update branch from 34903a6 to 691c71a Compare September 18, 2018 13:36

lkundrak merged commit 691c71a into master Sep 18, 2018

lkundrak deleted the bg/n-acd-update branch December 18, 2019 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bg/n-acd-update: update n-acd code from upstream #195

bg/n-acd-update: update n-acd code from upstream #195

bengal commented Sep 7, 2018

thom311 Sep 8, 2018

bengal Sep 13, 2018

thom311 Sep 8, 2018

bengal Sep 13, 2018

thom311 Sep 8, 2018

bengal Sep 13, 2018

thom311 Sep 8, 2018

thom311 Sep 8, 2018

thom311 Sep 8, 2018

thom311 Sep 8, 2018

bengal Sep 13, 2018

thom311 Sep 17, 2018

thom311 Sep 17, 2018

thom311 Sep 8, 2018

bengal Sep 13, 2018

bengal commented Sep 13, 2018

thom311 Sep 17, 2018

bengal Sep 18, 2018

thom311 left a comment

		@@ -117,21 +118,15 @@ static const char *
		_acd_error_to_string (int error)

bg/n-acd-update: update n-acd code from upstream #195

bg/n-acd-update: update n-acd code from upstream #195

Conversation

bengal commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengal commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thom311 left a comment

Choose a reason for hiding this comment