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
base: master
from

Conversation

Projects
None yet
@jplitza
Member

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.

@NeoRaider NeoRaider added this to the next milestone Jul 30, 2016

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Jul 31, 2016

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.

Contributor

RalfJung commented Jul 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Aug 1, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Aug 1, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Aug 1, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Aug 1, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Aug 1, 2016

Member

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.

Member

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.

@NeoRaider

This comment has been minimized.

Show comment
Hide comment
@NeoRaider

NeoRaider Sep 8, 2016

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.

Member

NeoRaider commented Sep 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Sep 28, 2016

Member

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?

Member

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

Add package gluon-radv-filterd
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

This comment has been minimized.

Show comment
Hide comment
@jjsarton

jjsarton Oct 24, 2016

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.

jjsarton commented Oct 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Oct 25, 2016

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@jjsarton

jjsarton Oct 25, 2016

@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 ?

jjsarton commented Oct 25, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Oct 25, 2016

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@A-Kasper

A-Kasper Oct 25, 2016

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?

Contributor

A-Kasper commented Oct 25, 2016

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 added a commit to freifunk-mwu/packages-ffmwu that referenced this pull request Nov 23, 2016

Add package gluon-radv-filterd
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

This comment has been minimized.

Show comment
Hide comment
@belzebub40k

belzebub40k Nov 23, 2016

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.

Contributor

belzebub40k commented Nov 23, 2016

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.

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Nov 24, 2016

Member

@belzebub40k @rotanid If I understand correctly, the patch is needed for batman-adv 2016.2, not for Gluon 2016.2, right? So the tag "needs rebase" is not really appropriate.

Member

jplitza commented Nov 24, 2016

@belzebub40k @rotanid If I understand correctly, the patch is needed for batman-adv 2016.2, not for Gluon 2016.2, right? So the tag "needs rebase" is not really appropriate.

@rotanid

This comment has been minimized.

Show comment
Hide comment
@rotanid

rotanid Nov 24, 2016

Member

at first i thought he meant the gluon version, really annoying if people aren't precise in that regard.
but, @jplitza as the current master gluon branch and its documentation lead to a build running with batman-adv 2016.2, this has to be changed anyway to support batman-adv 2016.2 (or newer), too. even if some people are still running older batman-adv versions with the current gluon, this is not the default. i agree though, that it's not a "rebase" per se.

Member

rotanid commented Nov 24, 2016

at first i thought he meant the gluon version, really annoying if people aren't precise in that regard.
but, @jplitza as the current master gluon branch and its documentation lead to a build running with batman-adv 2016.2, this has to be changed anyway to support batman-adv 2016.2 (or newer), too. even if some people are still running older batman-adv versions with the current gluon, this is not the default. i agree though, that it's not a "rebase" per se.

@rotanid rotanid added needs work and removed needs rebase labels Nov 24, 2016

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Nov 24, 2016

Member

Yes, this needs work in that regard, and I already commented on that in the commit that @belzebub40k linked to. The solution probably is to call sscanf() twice with different format strings.

Member

jplitza commented Nov 24, 2016

Yes, this needs work in that regard, and I already commented on that in the commit that @belzebub40k linked to. The solution probably is to call sscanf() twice with different format strings.

@belzebub40k

This comment has been minimized.

Show comment
Hide comment
@belzebub40k

belzebub40k Nov 25, 2016

Contributor

@jplitza @rotanid yes, I was referring to batman-adv 2016.2

Contributor

belzebub40k commented Nov 25, 2016

@jplitza @rotanid yes, I was referring to batman-adv 2016.2

@belzebub40k

This comment has been minimized.

Show comment
Hide comment
@belzebub40k
Contributor

belzebub40k commented Nov 25, 2016

@rotanid rotanid removed the needs work label Nov 26, 2016

if (router->tq == 0)
break;
}
if (router != NULL) {

This comment has been minimized.

@belzebub40k

belzebub40k Nov 27, 2016

Contributor

Maybe I get something wrong as my C skills are not very good. But shouldn't this be inside the foreach loop after the "if" that breaks the loop? Or should there be a return instead of a break to stop the execution of the whole function?

@belzebub40k

belzebub40k Nov 27, 2016

Contributor

Maybe I get something wrong as my C skills are not very good. But shouldn't this be inside the foreach loop after the "if" that breaks the loop? Or should there be a return instead of a break to stop the execution of the whole function?

This comment has been minimized.

@jplitza

jplitza Nov 29, 2016

Member

Nope, it's correct as it is.

If the loop is executed without breaking, the variable router will point behind the end of the list, i.e. to NULL. Thus, this if translates to "if we called break in the loop…"

@jplitza

jplitza Nov 29, 2016

Member

Nope, it's correct as it is.

If the loop is executed without breaking, the variable router will point behind the end of the list, i.e. to NULL. Thus, this if translates to "if we called break in the loop…"

This comment has been minimized.

@belzebub40k

belzebub40k Nov 29, 2016

Contributor

Ok, so this foreach is for setting the pointer to the next router that has a TQ of 0. If no more routers have a TQ of 0 the pointer is set to NULL?

@belzebub40k

belzebub40k Nov 29, 2016

Contributor

Ok, so this foreach is for setting the pointer to the next router that has a TQ of 0. If no more routers have a TQ of 0 the pointer is set to NULL?

This comment has been minimized.

@jplitza

jplitza Nov 29, 2016

Member

Yes, you could say that.

@jplitza

jplitza Nov 29, 2016

Member

Yes, you could say that.

@rotanid

This comment has been minimized.

Show comment
Hide comment
@rotanid

rotanid Dec 8, 2016

Member

@belzebub40k are the CPU load issues gone? or do you suggest to implement something that you already did for your community?

Member

rotanid commented Dec 8, 2016

@belzebub40k are the CPU load issues gone? or do you suggest to implement something that you already did for your community?

@belzebub40k

This comment has been minimized.

Show comment
Hide comment
@belzebub40k

belzebub40k Dec 8, 2016

Contributor

@rotanid As i wrote on November 27th I've implemented the gateways table as primary TQ source which reduced the CPU load to <= 1%.

Contributor

belzebub40k commented Dec 8, 2016

@rotanid As i wrote on November 27th I've implemented the gateways table as primary TQ source which reduced the CPU load to <= 1%.

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Dec 9, 2016

Member

I just added a patch that uses fscanf() instead of getline() and sscanf(), reducing the CPU load from 3%-4% to steady 1% on my WDR4300. Also, I combined the two calls that I introduced to differentiate the batman-adv versions and fixed a use-after-free bug.

@belzebub40k Could you please test the new version if CPU load is acceptable now and if it still works with batman-adv 2016.2?

Update: Actually, if we are discussing CPU strain, the most obvious thing is to increase the interval at which the originators and TQs are checked. I just did that (previously 5s, now 15s).

Member

jplitza commented Dec 9, 2016

I just added a patch that uses fscanf() instead of getline() and sscanf(), reducing the CPU load from 3%-4% to steady 1% on my WDR4300. Also, I combined the two calls that I introduced to differentiate the batman-adv versions and fixed a use-after-free bug.

@belzebub40k Could you please test the new version if CPU load is acceptable now and if it still works with batman-adv 2016.2?

Update: Actually, if we are discussing CPU strain, the most obvious thing is to increase the interval at which the originators and TQs are checked. I just did that (previously 5s, now 15s).

@rotanid

This comment has been minimized.

Show comment
Hide comment
@rotanid

rotanid Dec 10, 2016

Member

short sidenote regarding the batman-adv versions:
there may also be changes between 2016.2 and 2016.4, if we don't take care of this now we should at least keep it in mind.
at least the output of batctl changed between these two releases.

Member

rotanid commented Dec 10, 2016

short sidenote regarding the batman-adv versions:
there may also be changes between 2016.2 and 2016.4, if we don't take care of this now we should at least keep it in mind.
at least the output of batctl changed between these two releases.

@belzebub40k

This comment has been minimized.

Show comment
Hide comment
@belzebub40k

belzebub40k Dec 11, 2016

Contributor

@jplitza CPU load looks good but now I can see high memory usage (up to 7% on a WDR3600) every time the originators table is read. I can't remember seeing that with the previous version.

EDIT: The WDR3600 has 4 times the memory of a WR841 (32MB) and a usage of about 9MB (128MB*7%) could easy cause an OOM. Maybe we should wait for #976 to get merged to avoid reading the huge originators table. (#976 has been merged)

Contributor

belzebub40k commented Dec 11, 2016

@jplitza CPU load looks good but now I can see high memory usage (up to 7% on a WDR3600) every time the originators table is read. I can't remember seeing that with the previous version.

EDIT: The WDR3600 has 4 times the memory of a WR841 (32MB) and a usage of about 9MB (128MB*7%) could easy cause an OOM. Maybe we should wait for #976 to get merged to avoid reading the huge originators table. (#976 has been merged)

@belzebub40k

This comment has been minimized.

Show comment
Hide comment
@belzebub40k

belzebub40k Dec 12, 2016

Contributor

There seems to a problem on some targets. When I try the package on x86-64 (virtualbox) or mvebu (Linksys WRT1200AC) I get the following message (with DEBUG) and ebtables only has a accept all rule.

daemon.err gluon-radv-filterd[2586]: select() timeout expired

Contributor

belzebub40k commented Dec 12, 2016

There seems to a problem on some targets. When I try the package on x86-64 (virtualbox) or mvebu (Linksys WRT1200AC) I get the following message (with DEBUG) and ebtables only has a accept all rule.

daemon.err gluon-radv-filterd[2586]: select() timeout expired

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Dec 13, 2016

Member

Snap. Let me guess, mvebu is 64bit?

Apparently, the pcap rules that need to be applied to the socket to filter out router advertisements are a little different there. On ar71xx (and in the code line 189), the next-to-last line reads

{ 0x06, 0, 0, 0x0000ffff },

whereas on x86_64, it reads

{ 0x06, 0, 0, 0x00040000 },

Could you please run the following on a node with mvebu and compare the output of the tcpdump call with the lines 179–190?

opkg update
opkg install -d ram tcpdump ip-full
/tmp/usr/bin/ip tuntap add dev tun mode tun
ip link set up tun
LD_LIBRARY_PATH=/tmp/usr/lib /tmp/usr/sbin/tcpdump -i tun "icmp6 and ip6[40] = 134" -dd

I'll probably have to understand how these filters work in more depth to understand how they can be made platform-independent. Any help is appreciated.

Member

jplitza commented Dec 13, 2016

Snap. Let me guess, mvebu is 64bit?

Apparently, the pcap rules that need to be applied to the socket to filter out router advertisements are a little different there. On ar71xx (and in the code line 189), the next-to-last line reads

{ 0x06, 0, 0, 0x0000ffff },

whereas on x86_64, it reads

{ 0x06, 0, 0, 0x00040000 },

Could you please run the following on a node with mvebu and compare the output of the tcpdump call with the lines 179–190?

opkg update
opkg install -d ram tcpdump ip-full
/tmp/usr/bin/ip tuntap add dev tun mode tun
ip link set up tun
LD_LIBRARY_PATH=/tmp/usr/lib /tmp/usr/sbin/tcpdump -i tun "icmp6 and ip6[40] = 134" -dd

I'll probably have to understand how these filters work in more depth to understand how they can be made platform-independent. Any help is appreciated.

@jplitza jplitza added the needs work label Dec 13, 2016

ecsv added some commits Dec 20, 2017

gluon-radv-filterd: Don't kill daemon when select is interrupted
The select can be interrupted when it receives a signal. But the signal
might be handled and thus it should not result in an kill.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Reset chain when daemon shuts down
The daemon must make sure that it doesn't filter any incoming router
advertisement when it was shut down. This can be achieved by flushing all
current rules and/or adding an ACCEPT all rule at the end. When both
commands work, the state of the chain will be the same as
/lib/gluon/ebtables/400-radv-filter created it.

This doesn't handle the problem that the daemon may have been crashed and
thus the chain is in an undefined state.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Initialize router->originator after alloc
The memory returned after malloc is not initialized. It must be initialized
before it is accessed in update_tqs and compared against 00:00:00:00:00:00.
Otherwise the TQ retrievel could fail because the originator address is
never updated.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Use generic netlink to request batman-adv data
The correct way to get the data from batman-adv is not to try to parse the
freeform debugfs files. Instead, the generic netlink family "batadv" should
be used to request the tables in binary form.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Fix sock initialization check
A socket with the value 0 is valid (and it the first opened socket). It is
therefore a bad idea to check for 0 when wanting to find out whether a
socket was initialized.

Instead initialize it with -1 and check for < 0 to find out whether the
socket was initialized or not.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Trigger config reload checks on interface.* events
The init scripts adds the br-client as netdev for the daemon. The daemon
will automatically be restarted when the netdev's ifindex is changed and
the reload target of the init script is called. But something has to call
this script first.

This can be done the procd triggers interface which can simply wait for all
events from type "interface.*". The reload target will always be called but
the daemon will only be restarted when the br-client ifindex actually
changed.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
gluon-radv-filterd: Call cleanup when stopping daemon
Signed-off-by: Sven Eckelmann <sven@narfation.org>
@ecsv

This comment has been minimized.

Show comment
Hide comment
@ecsv

ecsv Jan 3, 2018

Contributor

The new patches from @jplitza are now missing:

Contributor

ecsv commented Jan 3, 2018

The new patches from @jplitza are now missing:

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Jan 3, 2018

Member

Rebased to master and cherry-picked almost all commits by @ecsv. I skipped those regarding ebtables --concurrent, as they change stuff outside of this package and are beneficial to other packages as well. As such, I think they should have their own PR.

Thanks so much for your work on this, @ecsv!

Member

jplitza commented Jan 3, 2018

Rebased to master and cherry-picked almost all commits by @ecsv. I skipped those regarding ebtables --concurrent, as they change stuff outside of this package and are beneficial to other packages as well. As such, I think they should have their own PR.

Thanks so much for your work on this, @ecsv!

@lemoer

This comment has been minimized.

Show comment
Hide comment
@lemoer

lemoer Jan 3, 2018

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@lemoer

lemoer Jan 3, 2018

Contributor

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.

Contributor

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.

@NeoRaider

This comment has been minimized.

Show comment
Hide comment
@NeoRaider

NeoRaider Jan 3, 2018

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).

Member

NeoRaider commented Jan 3, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@lemoer

lemoer Jan 3, 2018

Contributor

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

Contributor

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 some commits Dec 20, 2017

gluon-radv-filterd: Use ebtables locking
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

This comment has been minimized.

Show comment
Hide comment
@rotanid

rotanid Jan 3, 2018

Member

@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?

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?

@ecsv

ecsv approved these changes Jan 4, 2018

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Jan 4, 2018

Member

@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.

Member

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.

@ecsv

This comment has been minimized.

Show comment
Hide comment
@ecsv

ecsv Jan 5, 2018

Contributor

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");
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

This comment has been minimized.

Show comment
Hide comment
@lemoer

lemoer Jan 5, 2018

Contributor

@mweinelt and me brainstormed a little on the documentation.

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

Contributor

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 referenced this pull request Jan 19, 2018

Open

IPv6 gateway selection #277

@mweinelt

This comment has been minimized.

Show comment
Hide comment
@mweinelt

mweinelt Jan 24, 2018

Contributor

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
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

This comment has been minimized.

Show comment
Hide comment
@lemoer

lemoer Jan 24, 2018

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@lemoer
Contributor

lemoer commented Jan 25, 2018

@NeoRaider

This comment has been minimized.

Show comment
Hide comment
@NeoRaider

NeoRaider Jan 25, 2018

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!

Member

NeoRaider commented Jan 25, 2018

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!

@NeoRaider NeoRaider closed this Jan 25, 2018

@NeoRaider NeoRaider deleted the radv-filterd branch Jan 25, 2018

@mortzel mortzel referenced this pull request Jan 26, 2018

Closed

Add gluon-radv-filterd #148

@jplitza

This comment has been minimized.

Show comment
Hide comment
@jplitza

jplitza Jan 29, 2018

Member

Thanks to everybody who made this merge possible! 😄

Member

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