From: Roy Marples <roy@marples.name>
Date: Tue, 24 Oct 2017 22:11:15 +0000 (+0100)
Subject: bpf: store flags in state for a better abort
X-Git-Tag: v7.0.0-rc4~17
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9bfb04f6c48aa1265fb3f026a4d4095f67b08f54;p=thirdparty%2Fdhcpcd.git

bpf: store flags in state for a better abort

There are cases when dhcp may close and open an ARP socket
during an ARP read. This means the fd will not be -1, so we need
to set BPF_EOF when closing the socket.
---

diff --git a/src/arp.c b/src/arp.c
index 03e30b8c..fd3cdfe3 100644
--- a/src/arp.c
+++ b/src/arp.c
@@ -98,7 +98,7 @@ arp_request(const struct interface *ifp, in_addr_t sip, in_addr_t tip)
 	APPEND(&tip, sizeof(tip));
 
 	state = ARP_CSTATE(ifp);
-	return bpf_send(ifp, state->fd, ETHERTYPE_ARP, arp_buffer, len);
+	return bpf_send(ifp, state->bpf_fd, ETHERTYPE_ARP, arp_buffer, len);
 
 eexit:
 	errno = ENOBUFS;
@@ -177,10 +177,11 @@ arp_close(struct interface *ifp)
 {
 	struct iarp_state *state;
 
-	if ((state = ARP_STATE(ifp)) != NULL && state->fd != -1) {
-		eloop_event_delete(ifp->ctx->eloop, state->fd);
-		bpf_close(ifp, state->fd);
-		state->fd = -1;
+	if ((state = ARP_STATE(ifp)) != NULL && state->bpf_fd != -1) {
+		eloop_event_delete(ifp->ctx->eloop, state->bpf_fd);
+		bpf_close(ifp, state->bpf_fd);
+		state->bpf_fd = -1;
+		state->bpf_flags |= BPF_EOF;
 	}
 }
 
@@ -188,18 +189,18 @@ static void
 arp_read(void *arg)
 {
 	struct interface *ifp = arg;
-	const struct iarp_state *state;
+	struct iarp_state *state;
 	uint8_t buf[ARP_LEN];
-	int flags;
 	ssize_t bytes;
 
 	/* Some RAW mechanisms are generic file descriptors, not sockets.
 	 * This means we have no kernel call to just get one packet,
 	 * so we have to process the entire buffer. */
-	state = ARP_CSTATE(ifp);
-	flags = 0;
-	while (!(flags & BPF_EOF)) {
-		bytes = bpf_read(ifp, state->fd, buf, sizeof(buf), &flags);
+	state = ARP_STATE(ifp);
+	state->bpf_flags &= ~BPF_EOF;
+	while (!(state->bpf_flags & BPF_EOF)) {
+		bytes = bpf_read(ifp, state->bpf_fd, buf, sizeof(buf),
+				 &state->bpf_flags);
 		if (bytes == -1) {
 			logerr("%s: %s", __func__, ifp->name);
 			arp_close(ifp);
@@ -207,7 +208,7 @@ arp_read(void *arg)
 		}
 		arp_packet(ifp, buf, (size_t)bytes);
 		/* Check we still have a state after processing. */
-		if ((state = ARP_CSTATE(ifp)) == NULL || state->fd == -1)
+		if ((state = ARP_STATE(ifp)))
 			break;
 	}
 }
@@ -218,15 +219,15 @@ arp_open(struct interface *ifp)
 	struct iarp_state *state;
 
 	state = ARP_STATE(ifp);
-	if (state->fd == -1) {
-		state->fd = bpf_open(ifp, bpf_arp);
-		if (state->fd == -1) {
+	if (state->bpf_fd == -1) {
+		state->bpf_fd = bpf_open(ifp, bpf_arp);
+		if (state->bpf_fd == -1) {
 			logerr("%s: %s", __func__, ifp->name);
 			return -1;
 		}
-		eloop_event_add(ifp->ctx->eloop, state->fd, arp_read, ifp);
+		eloop_event_add(ifp->ctx->eloop, state->bpf_fd, arp_read, ifp);
 	}
-	return state->fd;
+	return state->bpf_fd;
 }
 
 static void
@@ -273,7 +274,7 @@ arp_probe(struct arp_state *astate)
 	} else {
 		const struct iarp_state *state = ARP_CSTATE(astate->iface);
 
-		if (bpf_arp(astate->iface, state->fd) == -1)
+		if (bpf_arp(astate->iface, state->bpf_fd) == -1)
 			logerr(__func__);
 	}
 	astate->probes = 0;
@@ -451,7 +452,8 @@ arp_new(struct interface *ifp, const struct in_addr *addr)
 			logerr(__func__);
 			return NULL;
 		}
-		state->fd = -1;
+		state->bpf_fd = -1;
+		state->bpf_flags = 0;
 		TAILQ_INIT(&state->arp_states);
 	} else {
 		if (addr && (astate = arp_find(ifp, addr)))
@@ -468,7 +470,7 @@ arp_new(struct interface *ifp, const struct in_addr *addr)
 	state = ARP_STATE(ifp);
 	TAILQ_INSERT_TAIL(&state->arp_states, astate, next);
 
-	if (bpf_arp(ifp, state->fd) == -1)
+	if (bpf_arp(ifp, state->bpf_fd) == -1)
 		logerr(__func__); /* try and continue */
 
 	return astate;
@@ -504,7 +506,7 @@ arp_free(struct arp_state *astate)
 		free(state);
 		ifp->if_data[IF_DATA_ARP] = NULL;
 	} else
-		if (bpf_arp(ifp, state->fd) == -1)
+		if (bpf_arp(ifp, state->bpf_fd) == -1)
 			logerr(__func__);
 }
 
diff --git a/src/arp.h b/src/arp.h
index f2746965..9737ba9c 100644
--- a/src/arp.h
+++ b/src/arp.h
@@ -76,7 +76,8 @@ struct arp_state {
 TAILQ_HEAD(arp_statehead, arp_state);
 
 struct iarp_state {
-	int fd;
+	int bpf_fd;
+	unsigned int bpf_flags;
 	struct arp_statehead arp_states;
 };
 
diff --git a/src/bpf.c b/src/bpf.c
index a6b7bdb8..8efca21f 100644
--- a/src/bpf.c
+++ b/src/bpf.c
@@ -194,7 +194,8 @@ eexit:
 /* BPF requires that we read the entire buffer.
  * So we pass the buffer in the API so we can loop on >1 packet. */
 ssize_t
-bpf_read(struct interface *ifp, int fd, void *data, size_t len, int *flags)
+bpf_read(struct interface *ifp, int fd, void *data, size_t len,
+    unsigned int *flags)
 {
 	ssize_t fl = (ssize_t)bpf_frame_header_len(ifp);
 	ssize_t bytes;
@@ -203,7 +204,7 @@ bpf_read(struct interface *ifp, int fd, void *data, size_t len, int *flags)
 	struct bpf_hdr packet;
 	const char *payload;
 
-	*flags = 0;
+	*flags &= ~BPF_EOF;
 	for (;;) {
 		if (state->buffer_len == 0) {
 			bytes = read(fd, state->buffer, state->buffer_size);
diff --git a/src/bpf.h b/src/bpf.h
index 0af94f05..89e69bca 100644
--- a/src/bpf.h
+++ b/src/bpf.h
@@ -28,8 +28,8 @@
 #ifndef BPF_HEADER
 #define BPF_HEADER
 
-#define BPF_EOF			1 << 0
-#define BPF_PARTIALCSUM		2 << 0
+#define BPF_EOF			(1U << 0)
+#define BPF_PARTIALCSUM		(2U << 0)
 
 #include "dhcpcd.h"
 
@@ -39,7 +39,7 @@ int bpf_open(struct interface *, int (*)(struct interface *, int));
 int bpf_close(struct interface *, int);
 int bpf_attach(int, void *, unsigned int);
 ssize_t bpf_send(const struct interface *, int, uint16_t, const void *, size_t);
-ssize_t bpf_read(struct interface *, int, void *, size_t, int *);
+ssize_t bpf_read(struct interface *, int, void *, size_t, unsigned int *);
 int bpf_arp(struct interface *, int);
 int bpf_bootp(struct interface *, int);
 #endif
diff --git a/src/dhcp.c b/src/dhcp.c
index b138a6fc..1e93078d 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -1551,6 +1551,7 @@ dhcp_close(struct interface *ifp)
 		eloop_event_delete(ifp->ctx->eloop, state->bpf_fd);
 		bpf_close(ifp, state->bpf_fd);
 		state->bpf_fd = -1;
+		state->bpf_flags |= BPF_EOF;
 	}
 
 	state->interval = 0;
@@ -3236,14 +3237,15 @@ valid_udp_packet(void *data, size_t data_len, struct in_addr *from,
 }
 
 static void
-dhcp_handlepacket(struct interface *ifp, uint8_t *data, size_t len, int flags)
+dhcp_handlepacket(struct interface *ifp, uint8_t *data, size_t len)
 {
 	struct bootp *bootp;
 	struct in_addr from;
 	size_t udp_len;
 	const struct dhcp_state *state = D_CSTATE(ifp);
 
-	if (valid_udp_packet(data, len, &from, flags & RAW_PARTIALCSUM) == -1)
+	if (valid_udp_packet(data, len, &from,
+			     state->bpf_flags & RAW_PARTIALCSUM) == -1)
 	{
 		if (errno == EINVAL)
 			logerrx("%s: UDP checksum failure from %s",
@@ -3292,15 +3294,15 @@ dhcp_readpacket(void *arg)
 	struct interface *ifp = arg;
 	uint8_t buf[MTU_MAX];
 	ssize_t bytes;
-	int flags;
-	const struct dhcp_state *state = D_CSTATE(ifp);
+	struct dhcp_state *state = D_STATE(ifp);
 
 	/* Some RAW mechanisms are generic file descriptors, not sockets.
 	 * This means we have no kernel call to just get one packet,
 	 * so we have to process the entire buffer. */
-	flags = 0;
-	while (!(flags & BPF_EOF)) {
-		bytes = bpf_read(ifp, state->bpf_fd, buf, sizeof(buf), &flags);
+	state->bpf_flags &= ~BPF_EOF;
+	while (!(state->bpf_flags & BPF_EOF)) {
+		bytes = bpf_read(ifp, state->bpf_fd, buf, sizeof(buf),
+				 &state->bpf_flags);
 		if (bytes == -1) {
 			if (state->state != DHS_NONE) {
 				logerr("%s: %s", __func__, ifp->name);
@@ -3308,9 +3310,9 @@ dhcp_readpacket(void *arg)
 			}
 			return;
 		}
-		dhcp_handlepacket(ifp, buf, (size_t)bytes, flags);
+		dhcp_handlepacket(ifp, buf, (size_t)bytes);
 		/* Check we still have a state after processing. */
-		if ((state = D_CSTATE(ifp)) == NULL || state->bpf_fd == -1)
+		if ((state = D_STATE(ifp)) == NULL)
 			break;
 	}
 }
diff --git a/src/dhcp.h b/src/dhcp.h
index 08203dcc..134aee26 100644
--- a/src/dhcp.h
+++ b/src/dhcp.h
@@ -215,6 +215,7 @@ struct dhcp_state {
 	int socket;
 
 	int bpf_fd;
+	unsigned int bpf_flags;
 	struct ipv4_addr *addr;
 	uint8_t added;
 
diff --git a/src/if-linux.c b/src/if-linux.c
index b5e30eb9..0d49ab6b 100644
--- a/src/if-linux.c
+++ b/src/if-linux.c
@@ -1385,7 +1385,8 @@ eexit:
 /* BPF requires that we read the entire buffer.
  * So we pass the buffer in the API so we can loop on >1 packet. */
 ssize_t
-bpf_read(struct interface *ifp, int s, void *data, size_t len, int *flags)
+bpf_read(struct interface *ifp, int s, void *data, size_t len,
+    unsigned int *flags)
 {
 	ssize_t bytes;
 	struct ipv4_state *state = IPV4_STATE(ifp);
@@ -1409,7 +1410,8 @@ bpf_read(struct interface *ifp, int s, void *data, size_t len, int *flags)
 	bytes = recvmsg(s, &msg, 0);
 	if (bytes == -1)
 		return -1;
-	*flags = BPF_EOF; /* We only ever read one packet. */
+	*flags |= BPF_EOF; /* We only ever read one packet. */
+	*flags &= ~BPF_PARTIALCSUM;
 	if (bytes) {
 		ssize_t fl = (ssize_t)bpf_frame_header_len(ifp);