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

gluon-mesh-batman-adv: Do not add local-if to respondd #2147

Closed

Conversation

SmithChart
Copy link
Contributor

If there are local services running on an interface that sits on top of bat0
these interfaces may be added to the translocal batman-table. Contrary
to the bat0-interface itself this entries are not marked with the
TT_CLIENT_NOPURGE -flag and are thus added to the client-count reported by
respondd. In this case the client-count reported is one too high.

This change adds an additional check to the function doing the actual
counting. This check makes sure that no local interface is counted as a
client.

Since respondd only collects data in an interval of minutes (depending
on the site's configuration) the additional costs for this check should
be negligible.

This problem appeared during development of the Project Parker mesh-vpn implementation. In this scenario br-client is configured with an IP address:

19: br-client: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 72:4c:49:0a:78:e9 brd ff:ff:ff:ff:ff:ff
    inet 10.0.224.1/22 brd 10.0.227.255 scope global br-client
       valid_lft forever preferred_lft forever
    inet6 2001:bf7:381:38::1/64 scope global
       valid_lft forever preferred_lft forever
    inet6 fe80::704c:49ff:fe0a:78e9/64 scope link
       valid_lft forever preferred_lft forever

and services like DHCPD and RA are running on this interface.

If there are local services running on an interface that sits on top of bat0
these interfaces may be added to the translocal batman-table. Contrary
to the bat0-interface itself this entries are not marked with the
TT_CLIENT_NOPURGE -flag and are thus added to the client-count reported by
respondd. In this case the client-count reported is one too high.

This change adds an additional check to the function doing the actual
counting. This check makes sure that no local interface is counted as a
client.

Since respondd only collects data in an interval of minutes (depending
on the site's configuration) the additional costs for this check should
be negligible.
@mweinelt
Copy link
Contributor

If there are local services running on an interface that sits on top of bat0
these interfaces may be added to the translocal batman-table.

Can you explain this setup more thoroughly? I added an IPv4 address to br-client, but that did not change the client count.

@SmithChart
Copy link
Contributor Author

SmithChart commented Nov 15, 2020

If there are local services running on an interface that sits on top of bat0
these interfaces may be added to the translocal batman-table.

Can you explain this setup more thoroughly? I added an IPv4 address to br-client, but that did not change the client count.

I am not really sure if adding an IP address is really enough. In our setup there is also an dnsmasq and an uradvd running on that interface. As far as I understand this leads to packages passing through the BATMAN-interface that carry the MAC-address of this interface what than leads to BATMAN learning about this MAC.

batctl tl looks like:

root@ci841v11:/# batctl tl
[B.A.T.M.A.N. adv openwrt-2019.2-9, MainIF/MAC: primary0/72:4c:49:0a:78:eb (bat0/98:de:d0:b4:e8:5c BATMAN_IV), TTVN: 4]
Client             VID Flags    Last seen (CRC       )
33:33:ff:0a:78:e9   -1 [.P....]   0.000   (0xa48c9389)
33:33:00:00:00:02   -1 [.P....]   0.000   (0xa48c9389)
33:33:ff:6a:1c:27   -1 [.P....]   0.000   (0xa48c9389)
       ffci1-bat0    1 [.P....]   0.000   (0xbed72805)
33:33:ff:00:00:01   -1 [.P....]   0.000   (0xa48c9389)
    ffci-brclient   -1 [......]   0.560   (0xa48c9389)
33:33:ff:1c:88:84   -1 [.P....]   0.000   (0xa48c9389)
       ffci1-bat0    0 [.P....]   0.000   (0xf7eb5522)
33:33:00:00:00:fb   -1 [.P....]   0.000   (0xa48c9389)
33:33:00:02:10:01   -1 [.P....]   0.000   (0xa48c9389)
01:00:5e:00:00:01   -1 [.P....]   0.000   (0xa48c9389)
33:33:ff:00:00:00   -1 [.P....]   0.000   (0xa48c9389)
       ffci1-bat0   -1 [.P....]   0.000   (0xa48c9389)
33:33:ff:00:00:7f   -1 [.P....]   0.000   (0xa48c9389)
33:33:00:00:00:01   -1 [.P....]   0.000   (0xa48c9389)

The line with ffci-brclient is the interface that we do not want to count since it is only the local bridge.

This behavior occurs in our v2020.2.1-based parker net with gluon-mesh-batman-adv-15 and BATMAN_IVbut not in our gluon-classic net with gluon-mesh-batman-adv-14 and BATMAN_IV_LEGACY.

@mweinelt
Copy link
Contributor

Okay, I cannot reproduce this on master, even when I try to ping from/to the IPv4 address. Did you also modify firewall settings in your project?

@SmithChart
Copy link
Contributor Author

Okay, I cannot reproduce this on master, even when I try to ping from/to the IPv4 address. Did you also modify firewall settings in your project?

Yes, we have quite a few changes to the firewall and overall network setup.

Regarding firewall there are two mayor steps to get from layer2-forwarding to layer3-routing:

First: Set the defaults for our mayor zones:
https://gitli.stratum0.org/ffbs/ffbs-packages/-/blob/next/gluon-ffbsnext-nodeconfig/files/lib/gluon/upgrade/997-parker-override-firewall

Second: Finetuning:
https://gitli.stratum0.org/ffbs/ffbs-packages/-/blob/next/gluon-ffbsnext-nodeconfig/files/lib/gluon/upgrade/904-parker-firewall

I am suprised: even without firewall (/etc/init.d/firewall stop) this does not happen on classic-gluon.

Copy link
Member

@blocktrron blocktrron left a comment

Choose a reason for hiding this comment

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

Little style nitpicks

Comment on lines +296 to +307
// Check if client is a local interface
tt_addr = nla_data(attrs[BATADV_ATTR_TT_ADDRESS]);
for(ifa = opts->ifaddr; ifa != NULL; ifa = ifa->ifa_next) {
if( (ifa->ifa_addr) && (ifa->ifa_addr->sa_family == AF_PACKET)) {
struct sockaddr_ll *s = (struct sockaddr_ll*) ifa->ifa_addr;
if(memcmp(s->sll_addr, tt_addr, ETH_ALEN) == 0) {
// This client is a local interface.
// Do not increment client count
return NL_OK;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

	// Check if client is a local interface
	tt_addr = nla_data(attrs[BATADV_ATTR_TT_ADDRESS]);
	for (ifa = opts->ifaddr; ifa; ifa = ifa->ifa_next) {
		if (!ifa->ifa_addr || ifa->ifa_addr->sa_family != AF_PACKET)
			continue;

		struct sockaddr_ll *s = (struct sockaddr_ll*) ifa->ifa_addr;
		if(!memcmp(s->sll_addr, tt_addr, ETH_ALEN)) {
			// This client is a local interface.
			// Do not increment client count
			return NL_OK;
		}
	}

Comment on lines +323 to +325
if (getifaddrs(& opts.ifaddr) == -1) {
opts.ifaddr = NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

Single statements are not wrapped in braces in this file. So remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a good idea in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am indifferent on this but I tend to go with @blocktrron for consistency in this file for now.

Comment on lines +331 to +333
if(opts.ifaddr != NULL) {
freeifaddrs(opts.ifaddr);
}
Copy link
Member

Choose a reason for hiding this comment

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

dito

@neocturne
Copy link
Member

br-client and bat0 always have the same MAC address in Gluon.

@SmithChart
Copy link
Contributor Author

br-client and bat0 always have the same MAC address in Gluon.

Ah. good point. Some time ago we decided that we want to set this address to something known:

root@parker-mesh-2:~# ip l
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether e6:41:5f:8e:b7:26 brd ff:ff:ff:ff:ff:ff
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master br-wan state UP mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:71 brd ff:ff:ff:ff:ff:ff
4: eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel state DOWN mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:72 brd ff:ff:ff:ff:ff:ff
6: br-client: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether a2:ea:21:09:90:79 brd ff:ff:ff:ff:ff:ff
7: eth1.1@eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue master br-client state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:72 brd ff:ff:ff:ff:ff:ff
8: br-wan: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:71 brd ff:ff:ff:ff:ff:ff
9: local-port@local-node: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br-client state UP mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:72 brd ff:ff:ff:ff:ff:ff
10: local-node@local-port: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 72:02:46:6a:1c:27 brd ff:ff:ff:ff:ff:ff
11: bat0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br-client state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether 64:66:b3:8e:bc:72 brd ff:ff:ff:ff:ff:ff
12: primary0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1532 qdisc noqueue master bat0 state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether a2:ea:21:09:90:7b brd ff:ff:ff:ff:ff:ff
13: mesh0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1532 qdisc noqueue master bat0 state UP mode DEFAULT group default qlen 1000
    link/ether a2:ea:21:09:90:79 brd ff:ff:ff:ff:ff:ff
14: client0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br-client state UP mode DEFAULT group default qlen 1000
    link/ether a2:ea:21:09:90:78 brd ff:ff:ff:ff:ff:ff
15: wg_c1: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1375 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/none 
16: wg_c2: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1375 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/none 
17: wg_c3: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1375 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/none 

I cannot remember why we decided to do so and our git history is not that conclusive:
https://gitli.stratum0.org/ffbs/ffbs-packages/-/commits/next/gluon-ffbsnext-nodeconfig/files/lib/gluon/upgrade/305-replace-br-client-mac

I will discuss this internally and update this PR once I have the information.

@SmithChart
Copy link
Contributor Author

I will discuss this internally and update this PR once I have the information.

It seems we don't need the self-set mac address anymore. I'll test that in the next days. And when I am successful I'll just close this PR.

@SmithChart
Copy link
Contributor Author

I will discuss this internally and update this PR once I have the information.

It seems we don't need the self-set mac address anymore. I'll test that in the next days. And when I am successful I'll just close this PR.

Thanks @NeoRaider! You guessed right. We have reverted our changes to the br-client mac-addr and now our client-count is fixed.
I'll close this PR since we do not need this extra logic anymore.

@SmithChart SmithChart closed this Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants