Skip to content
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.

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

Add package gluon-radv-filterd #838

Closed
wants to merge 51 commits into from
Closed

Add package gluon-radv-filterd #838

wants to merge 51 commits into from

Conversation

jplitza
Copy link
Member

@jplitza jplitza commented Jul 29, 2016

This package drops all incoming router advertisements except for the default router with the best metric according to B.A.T.M.A.N. advanced.

Note that advertisements originating from the node itself (for example via gluon-radvd) are not affected.

This PR supersedes #718.

Caveat: Router solicitations sent by clients are still broadcast, and all but one response are dropped. Making them unicast would require fiddling with the packet, which isn't easily possible from userspace as we've seen in #718.

@jplitza jplitza force-pushed the radv-filterd branch 4 times, most recently from 2aca457 to be91c62 Compare July 30, 2016 08:23
@neocturne neocturne added the 0. type: enhancement The changeset is an enhancement label Jul 30, 2016
@neocturne neocturne added this to the next milestone Jul 30, 2016
@jplitza jplitza force-pushed the radv-filterd branch 4 times, most recently from c6dcbe2 to 7053f2d Compare July 30, 2016 21:25
@RalfJung
Copy link
Contributor

except for the default router with the best metric according to B.A.T.M.A.N. advanced.

To clarify here: Is it picking the "default router" (as in, the "selected gateway" which B.A.T.M.A.N. advanced will also do its DHCP magic with) or the one with the best metric? The two are not necessarily the same; if the best metric is only slightly better than the selected GW then the selected GW does not change to avoid flapping between two GWs all the time.

@jplitza
Copy link
Member Author

jplitza commented Aug 1, 2016

I should probably improve the readme on that.

Yes, after @NeoRaider mentioned it on IRC I implemented a hysteresis, currently switching the selected gateway if ΔTQ≥20. This is comparable to the B.A.T.M.A.N. advanced algorithm with a selection class of 20, which is the default in Gluon AFAIK. But it doesn't use the B.A.T.M.A.N. advanced gateway selection, because that would require all gateways to handle both IPv4 and IPv6.

@RalfJung
Copy link
Contributor

RalfJung commented Aug 1, 2016

I see -- so this way, IPv4 gateways announce themselves as B.A.T.M.A.N. advanced gateways, while IPv6 gateways just send RAs to announce themselves? That makes sense, I guess, if properly documented. :)

Would it be possible to use the B.A.T.M.A.N. advanced selection class? Alternatively, at least, the 20 should be configurable.

@jplitza
Copy link
Member Author

jplitza commented Aug 1, 2016

Currently the ΔTQ threshold is a compile time constant. Would you rather have it as a site.conf variable?

Also, the other selection classes never struck me as very useful, as they stay with their initial choice of gateway forever/until that gateway vanishes. Thus, I'd rather not implement additional code to make them usable.

@RalfJung
Copy link
Contributor

RalfJung commented Aug 1, 2016

Oh, I wasn't even aware there are entirely different "classes" (as in, algorithms) -- but selecting a different threshold certainly sounds like something people may want to do. That threshold could be read from BATMAN, or I guess a site.conf variable also works. A compile-time constant sounds rather inflexible.

@jplitza
Copy link
Member Author

jplitza commented Aug 1, 2016

I now implemented the configuration of the threshold via site.conf (passed to UCI, passed to the command line). I also improved both the readme and the usage help. In turn, I removed the possibility to specify multiple interfaces, as that doesn't really make sense: We expect to find TQs from bat0 for every gateway, but that means we can only handle bat0 or a bridge on top of it as incoming interface.

@neocturne
Copy link
Member

I didn't get to have a closer look at this yet, but I usually try to avoid UCI for Gluon packages as much as possible, as each added option will be leftover configuration if the package is ever removed on a firmware upgrade.

@jplitza
Copy link
Member Author

jplitza commented Sep 28, 2016

The alternative would be to read stuff directly from the site.conf, right? That's not quite as easy using either bash/initscripts or C. Also, node owners cannot alter the behavior of the program that way. So how much are you opposed to using UCI?

jplitza added a commit to FreifunkBremen/ffhb-packages that referenced this pull request Oct 8, 2016
This is to be added upstream [1], but while that still takes time we want to
test and use it here already.

[1]: freifunk-gluon/gluon#838
@jjsarton
Copy link

According to my tests, batman-adv tell which gateway is selected (/sys/kernel/debug/batman_adv/bat0/gateways). while working with the datas returned we need only to read 2+2xgateway lines and can drop all ra's issued from gateways which are not marked as default.

@jplitza
Copy link
Member Author

jplitza commented Oct 25, 2016

@jjsarton Part of the design goal was to not rely on batman-adv's internal gateway selection, as that implies being an IPv4 router, which may not always be true for IPv6 routers. Furthermore, there already is a canonical way to announce IPv6 routing capability: Router Advertisements.

@jjsarton
Copy link

@jplitza I have read tje thread again and apparently the assumption ist that there are IPv4 only GW and IPv6 Only gateways. Is this true ?
On the other side BATMAN-ADV work on layer to and normally has no knowledge about IP. The routing datas and gateway choose relies mostly on MAC and TQ. Is this true ?

@jplitza
Copy link
Member Author

jplitza commented Oct 25, 2016

@jjsarton Yes, the assumption is that IPv4 gateways are not necessarily IPv6 gateways and vice versa. And yes, batman-adv operates on layer 2 and thus doesn't know anything about either IPv4 or IPv6. Its internal gateway selection feature depends on the (IPv4) gateways actively announcing in their batman-adv packets that they are (IPv4) gateways. But the semantics are for IPv4, not IPv6, and piggy-backing on this feature would make it impossible to have an IPv6 gateway without IPv4 connectivity or vice versa.

@A-Kasper
Copy link
Contributor

Am 08.09.2016 17:34 schrieb "Matthias Schiffer" notifications@github.com:

I didn't get to have a closer look at this yet, but I usually try to
avoid UCI for Gluon packages as much as possible, as each added option will
be leftover configuration if the package is ever removed on a firmware
upgrade.

I can't aggree to this philosophy. yes, you are right. there maybe some
unused config lines in the uci, but: who cares?! these are just a few
bytes. It's more important to be as flexible and modular as possible.

maybe we could add a naming convention for uci usage something like
package1.parameter1
...
package1.parameter10

package2.parameter1
...
package2.parameter10

...

package10.parameter1
...
package10.parameter10

and a section
package1.name = gluon_ radv_filterd
...

than you could set up to 10 custom packages in site.conf wich can be
cleaned up with every firmware update. you could also add some customizable
parameters, which just would stay after fw upgrade

would all be possible, but i ask me why we want to avoid this let it be 1
KB of maybe leftover data while just using uci.

why do you think it is important to save this small amount of config data?

belzebub40k pushed a commit to freifunk-mwu/packages-ffmwu that referenced this pull request Nov 23, 2016
This is to be added upstream [1], but while that still takes time we want to
test and use it here already.

[1]: freifunk-gluon/gluon#838
@belzebub40k
Copy link
Contributor

The version in the PR is not compatible with 2016.2 (and all other COMPAT 15 releases i guess).
I had to apply a small patch to make it work.

@rotanid rotanid added the 2. status: merge conflict The merge has a conflict and needs rebasing label Nov 23, 2016
@lemoer
Copy link
Member

lemoer commented Jan 3, 2018

I would suggest to add something like "CONFLICTS=gluon-ebtables-filter-ra-dhcp" to the Makefile. I'm not sure, whether there is such an option in OpenWRT Makefiles?

@lemoer
Copy link
Member

lemoer commented Jan 3, 2018

You should mention in your README, that only hosts which are actively sending router advertisements (with default routes! in) are considered as routers. So there is also no need in being a batman "gateway" (which refers to the ipv4 features of batman). Maybe you should also avoid calling the ipv6 routers as "gateways" in your README.

@neocturne
Copy link
Member

@lemoer CONFLICTS does exist in OpenWrt Makefiles, but I don't see why these packages should conflict (if there is a file or logical conflict at the moment, we should rather try to fix the conflict).

@lemoer
Copy link
Member

lemoer commented Jan 3, 2018

@NeoRaider You are right. I misinterpreted the README. Only in case you want to have local routers, you become in troubles.

ecsv and others added 2 commits January 3, 2018 22:12
This enables the ebtables internal locking mechanism which will avoid race
conditions between multiple, concurrent ebtables calls.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
@rotanid rotanid removed the needs work label Jan 3, 2018
@rotanid
Copy link
Member

rotanid commented Jan 3, 2018

@jplitza only after having documentation now i noticed the default of 20 for the TQ-difference that is necessary for a switch.
i don't know how realistic the scenario is, but shouldn't a gateway with 255/255 TQ always be preferred to one with lower TQ, even if the difference is smaller than 20?

@jplitza
Copy link
Member Author

jplitza commented Jan 4, 2018

@rotanid Remember that switching the gateway possibly incurs an IP address change in connected clients and thus shouldn't be done lightheartedly. Favoring a gateway with TQ 255 over one with TQ 254 (possibly only temporarily) isn't really an advantage worth the change.

Furthermore, I simply copied the default behavior for IPv4: Gluon sets the B.A.T.M.A.N. adv gateway selection class to 20 by default, which has the exact same semantics.


gluon_radv = {
threshold = 20,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

gluon_radv -> radv_filter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be radv_filterd according to the check_site.lua

Copy link
Member Author

Choose a reason for hiding this comment

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

Where was my mind…
Fixed.

site.conf
---------

radv_filter.threshold : optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be radv_filterd.threshold

@ecsv
Copy link
Contributor

ecsv commented Jan 5, 2018

Before I get angry mails: I don't want this to be integrated in this PR. I just wanted to post it where I (and other people) can find it again in the future. This message contains the result of my eBPF tests which I did together with gluon-radv-filterd


It might be required to to switch to eBPF at some point as shown in http://man7.org/linux/man-pages/man2/bpf.2.html. The kernel itself uses at the moment the eBPF JIT compiler/interpreter also for cBPF bytecode (https://cilium.readthedocs.io/en/latest/bpf/#bpf-and-xdp-reference-guide). But of course, cBPF bytecode doesn't use the extra (performance relevant and otherwise helpful) features which are provided by eBPF. Luckily, the program only requires a single register at the moment for the and thus doesn't gain much from the change at the moment (this might change in the future). It is therefore unlikely that we gain anything right now from switching to eBPF

The eBPF bytecode can be generated using

cat > test_ebpf.c << "EOF"
#include <linux/bpf.h>
#include <stddef.h>
#include <netinet/icmp6.h>
#include <netinet/in.h>
#include <netinet/ip6.h>

#ifndef __section
# define __section(NAME)                  \
   __attribute__((section(NAME), used))
#endif

struct sk_buff;
unsigned long long load_byte(void *skb,
			     unsigned long long off) asm("llvm.bpf.load.byte");
unsigned long long load_half(void *skb,
			     unsigned long long off) asm("llvm.bpf.load.half"); 

/* avoid extra &= 0xff, &=0xffff instructions when using u8/u16 types for loads */
typedef __u64 regtype;

__section("prog")
int check_radv(struct __sk_buff *skb)
{
	struct hdr {
		struct ip6_hdr ip6;
		struct nd_router_advert radv;
	};

	regtype next_hdr = load_byte(skb, offsetof(struct hdr, ip6.ip6_nxt));
	if (next_hdr != IPPROTO_ICMPV6)
		return 0;

	regtype icmp6_type = load_byte(skb, offsetof(struct hdr, radv.nd_ra_type));
	if (icmp6_type != ND_ROUTER_ADVERT)
		return 0;

	regtype ra_code = load_byte(skb, offsetof(struct hdr, radv.nd_ra_code));
	if (ra_code != 0)
		return 0;

	regtype lifetime = load_half(skb, offsetof(struct hdr, radv.nd_ra_router_lifetime));
	if (lifetime == 0)
		return 0;

	return sizeof(struct hdr);
}

char __license[] __section("license") = "GPL";
EOF
clang-5.0 -I/usr/include/x86_64-linux-gnu/ -I/usr/include/x86_64-linux-gnu -g -O2 -Wall -target bpf -x c -c test_ebpf.c -o test_ebpf.o
llvm-objdump-5.0 -S test_ebpf.o

The program can then use the bytecode by switching to SO_ATTACH_BPF and using the bpf syscall to send the bytecode to the kernel.:

diff --git a/package/gluon-radv-filterd/src/gluon-radv-filterd.c b/package/gluon-radv-filterd/src/gluon-radv-filterd.c
index c0a9d4af..083cdb5c 100644
--- a/package/gluon-radv-filterd/src/gluon-radv-filterd.c
+++ b/package/gluon-radv-filterd/src/gluon-radv-filterd.c
@@ -35,6 +35,7 @@
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <sys/syscall.h>
 
 #include <sys/socket.h>
 #include <sys/select.h>
@@ -44,6 +45,7 @@
 #include <net/ethernet.h>
 #include <net/if.h>
 
+#include <linux/bpf.h>
 #include <linux/filter.h>
 #include <linux/if_packet.h>
 #include <linux/limits.h>
@@ -99,6 +100,72 @@
 	     (item) && (((safe) = item->next) || 1); \
 	     item = safe)
 
+static __u64 ptr_to_u64(void *ptr)
+{
+	return (__u64) (unsigned long) ptr;
+}
+
+/* ALU ops on immediates, bpf_add|sub|...: dst_reg += imm32 */
+
+#define BPF_ALU64_IMM(OP, DST, IMM)				\
+	((struct bpf_insn) {					\
+		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_K,	\
+		.dst_reg = DST,					\
+		.src_reg = 0,					\
+		.off   = 0,					\
+		.imm   = IMM })
+
+/* Short form of mov, dst_reg = src_reg */
+
+#define BPF_MOV64_REG(DST, SRC)					\
+	((struct bpf_insn) {					\
+		.code  = BPF_ALU64 | BPF_MOV | BPF_X,		\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = 0,					\
+		.imm   = 0 })
+
+/* Short form of mov, dst_reg = imm32 */
+
+#define BPF_MOV64_IMM(DST, IMM)					\
+	((struct bpf_insn) {					\
+		.code  = BPF_ALU64 | BPF_MOV | BPF_K,		\
+		.dst_reg = DST,					\
+		.src_reg = 0,					\
+		.off   = 0,					\
+		.imm   = IMM })
+
+
+/* Direct packet access, R0 = *(uint *) (skb->data + imm32) */
+
+#define BPF_LD_ABS(SIZE, IMM)					\
+	((struct bpf_insn) {					\
+		.code  = BPF_LD | BPF_SIZE(SIZE) | BPF_ABS,	\
+		.dst_reg = 0,					\
+		.src_reg = 0,					\
+		.off   = 0,					\
+		.imm   = IMM })
+
+/* Conditional jumps against immediates, if (dst_reg 'op' imm32) goto pc + off16 */
+
+#define BPF_JMP_IMM(OP, DST, IMM, OFF)				\
+	((struct bpf_insn) {					\
+		.code  = BPF_JMP | BPF_OP(OP) | BPF_K,		\
+		.dst_reg = DST,					\
+		.src_reg = 0,					\
+		.off   = OFF,					\
+		.imm   = IMM })
+
+/* Program exit */
+
+#define BPF_EXIT_INSN()						\
+	((struct bpf_insn) {					\
+		.code  = BPF_JMP | BPF_EXIT,			\
+		.dst_reg = 0,					\
+		.src_reg = 0,					\
+		.off   = 0,					\
+		.imm   = 0 })
+
 struct router {
 	struct router *next;
 	struct ether_addr src;
@@ -216,35 +283,64 @@ static inline void warn_errno(const char *message) {
 	error_message(0, errno, "warning: %s", message);
 }
 
-static int init_packet_socket(unsigned int ifindex) {
-	struct sock_filter radv_filter_code[] = {
-		// check that this is an ICMPv6 packet
-		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, offsetof(struct ip6_hdr, ip6_nxt)),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, IPPROTO_ICMPV6, 0, 7),
-		// check that this is a router advertisement
-		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, sizeof(struct ip6_hdr) + offsetof(struct icmp6_hdr, icmp6_type)),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, ND_ROUTER_ADVERT, 0, 5),
-		// check that the code field in the ICMPv6 header is 0
-		BPF_STMT(BPF_LD|BPF_B|BPF_ABS, sizeof(struct ip6_hdr) + offsetof(struct nd_router_advert, nd_ra_code)),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, 0, 0, 3),
-		// check that this is a default route (lifetime > 0)
-		BPF_STMT(BPF_LD|BPF_H|BPF_ABS, sizeof(struct ip6_hdr) + offsetof(struct nd_router_advert, nd_ra_router_lifetime)),
-		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, 0, 1, 0),
-		// return true
-		BPF_STMT(BPF_RET|BPF_K, 0xffffffff),
-		// return false
-		BPF_STMT(BPF_RET|BPF_K, 0),
+static int init_bdf(void)
+{
+	struct hdr {
+		struct ip6_hdr ip6;
+		struct nd_router_advert radv;
+	};
+
+	struct bpf_insn radv_filter_code[] = {
+		// {
+		BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+		BPF_MOV64_IMM(BPF_REG_7, 0),
+		// __u8 next_hdr = load_byte(skb, offsetof(struct hdr, ip6.ip6_nxt));
+		BPF_LD_ABS(BPF_B, offsetof(struct hdr, ip6.ip6_nxt)),
+		// if (next_hdr != IPPROTO_ICMPV6)
+		BPF_JMP_IMM(BPF_JNE, BPF_REG_0, IPPROTO_ICMPV6, 8),
+		// __u8 icmp6_type = load_byte(skb, offsetof(struct hdr, radv.nd_ra_type));
+		BPF_LD_ABS(BPF_B, offsetof(struct hdr, radv.nd_ra_type)),
+		// if (icmp6_type != ND_ROUTER_ADVERT)
+		BPF_JMP_IMM(BPF_JNE, BPF_REG_0, ND_ROUTER_ADVERT, 6),
+		// __u8 ra_code = load_byte(skb, offsetof(struct hdr, radv.nd_ra_code));
+		BPF_LD_ABS(BPF_B, offsetof(struct hdr, radv.nd_ra_code)),
+		// if (ra_code != 0)
+		BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 4),
+		// __u16 lifetime = load_half(skb, offsetof(struct hdr, radv.nd_ra_router_lifetime));
+		BPF_LD_ABS(BPF_H, offsetof(struct hdr, radv.nd_ra_router_lifetime)),
+		// if (lifetime == 0)
+		//     return 0;
+		BPF_MOV64_IMM(BPF_REG_7, 0),
+		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+		// return sizeof(struct hdr);
+		BPF_MOV64_IMM(BPF_REG_7, sizeof(struct hdr)),
+		// }
+		BPF_MOV64_REG(BPF_REG_0, BPF_REG_7),
+		BPF_EXIT_INSN(),
 	};
 
-	struct sock_fprog radv_filter = {
-	    .len = ARRAY_SIZE(radv_filter_code),
-	    .filter = radv_filter_code,
+	union bpf_attr attr = {
+		.prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
+		.insns = ptr_to_u64((void *) radv_filter_code),
+		.insn_cnt = ARRAY_SIZE(radv_filter_code),
+		.license = ptr_to_u64((void *) "GPL"),
+		.log_buf = 0,
+		.log_size = 0,
+		.log_level = 0,
 	};
 
+	return syscall(__NR_bpf, BPF_PROG_LOAD, &attr, sizeof(attr));
+}
+
+static int init_packet_socket(unsigned int ifindex) {
+	int bdf_fd = init_bdf();
+	if (bdf_fd < 0)
+		exit_errno("can't load bdf filter");
+
 	int sock = socket(AF_PACKET, SOCK_DGRAM|SOCK_CLOEXEC, htons(ETH_P_IPV6));
 	if (sock < 0)
 		exit_errno("can't open packet socket");
-	int ret = setsockopt(sock, SOL_SOCKET, SO_ATTACH_FILTER, &radv_filter, sizeof(radv_filter));
+	int ret = setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &bdf_fd, sizeof(bdf_fd));
 	if (ret < 0)
 		exit_errno("can't attach socket filter");

@lemoer
Copy link
Member

lemoer commented Jan 5, 2018

@mweinelt and me brainstormed a little on the documentation.

Here is the result as a patch: https://paste.irrelefant.net/aicai0Of.txt

@rotanid rotanid mentioned this pull request Jan 19, 2018
@mweinelt
Copy link
Contributor

mweinelt commented Jan 24, 2018

Build fails for me when merged against the current master.

cp -fpR /scratch/hexa/build/gluon/lede/build_dir/target-mips_24kc_musl-1.1.16/root-ar71xx /scratch/hexa/build/gluon/lede/build_dir/target-mips_24kc_musl-1.1.16/root.orig-ar71xx
/scratch/hexa/build/gluon/lede/staging_dir/hostpkg/bin/lua: ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:62: bad argument #1 to 'unpack' (table expected, got string)
stack traceback:
        [C]: in function 'unpack'
        ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:62: in function 'loadvar'
        ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:83: in function <...atch/hexa/build/gluon/lede/../scripts/check_site.lua:82>
        (tail call): ?
        ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:150: in function 'need_table'
        stdin:1: in main chunk
        [C]: in function 'dofile'
        ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:187: in main chunk
        [C]: ?
postinst script ./usr/lib/opkg/info/gluon-radv-filterd.postinst has failed with exit code 1

@lemoer
Copy link
Member

lemoer commented Jan 24, 2018

Besides this build issue (most likely introduced by the multi domain preparations on the master branch) what's blocking the merge?

@lemoer
Copy link
Member

lemoer commented Jan 25, 2018

Fixup for @mweinelt 's build issue: https://paste.irrelefant.net/Aetiesi0.txt

@neocturne
Copy link
Member

Merged with the following changes:

  • rabased onto master (updated check_site script)
  • fixed documentation compile warnings
  • removed UCI config (the UCI config was overwritten from site.conf on each update, so having it was not very useful), removed upgrade script, removed multi-instance code from init script

Thanks!

@neocturne neocturne closed this Jan 25, 2018
@neocturne neocturne deleted the radv-filterd branch January 25, 2018 22:08
@jplitza
Copy link
Member Author

jplitza commented Jan 29, 2018

Thanks to everybody who made this merge possible! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. type: enhancement The changeset is an enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.