W dniu 11.03.2019 o 09:29, Otto Moerbeek pisze:
On Sun, Mar 10, 2019 at 02:41:35PM +0100, Michał Koc wrote:
W dniu 10.03.2019 o 08:10, Otto Moerbeek pisze:
On Sat, Mar 09, 2019 at 11:19:11PM +0100, Michał Koc wrote:
W dniu 09.03.2019 o 12:43, Otto Moerbeek pisze:
On Sat, Mar 09, 2019 at 12:10:34PM +0100, Michał Koc wrote:
W dniu 09.03.2019 o 08:15, Otto Moerbeek pisze:
On Fri, Mar 08, 2019 at 12:03:25PM +0100, Michał Koc wrote:
Hi all,
We have a triple redundant vpn gateway setup with sasyncd running and tons
of tunnels, about 1000 flows.
Looking at the graph of memory usage, you can clearly see that something is
sucking up the memory.
The graph can be viewed here: https://pasteboard.co/I4sjzQ8.jpg
Looking at the ps, sasyncd shows huge memory consumption:
USER PID %CPU %MEM VSZ RSS TT STAT
STARTED TIME COMMAND
_isakmpd 33560 0.0 17.0 699264 708508 ?? S
26Feb19 6:58.81 /usr/sbin/sasyncd
It only happens on the master node. Slaves do not show such a behavior.
There is nothing about sasyncd in the logs.
After sasyncd restart memory consumption is minimal, but tends to grow.
Is it normal ? or am I missing something ?
Best regards
M.K.
This is not normal. You could try to run with -vv to see if some error
path is taken that triggers a leak.
-Otto
Should I look for something specific ?
The log grows pretty fast and it looks like it could contain some security
data which I wouldn't like to post online.
The statistics of the log(about 2 hours) looks like this:
carp_init: 1
config: 7
monitor_get_pfkey_snap: 4
monitor_loop: 1
net: 1
net_connect: 3
net_ctl: 4
net_disconnect_peer: 3
net_handle_messages: 2
net_queue: 91780
net_read: 10
net_send_messages: 39192
pfkey_send_flush: 4
pfkey_snapshot: 6832
timer_add: 19
timer_run: 18
Best regards
M.K.
Just the counts does not reveal anything. I did a quick audit of the
memory allocation logic of sasyncd and did not spot an error. If you
do not want to post the logs, you'll neeed to analyze them yourself.
This requires matching the log lines to the code and tracking where
stuff gets allocated and deallocated. Some digging could reveal the error.
I used to run sasyncd, but I do no longer. Settig up a test env is
quite some work I do not have time for.
-Otto
First of all, thank You for your time. I know it's one of the most valuable
resource.
We have done some analysis and we have found the problem.
The problem is that the very master machine exists as a peer in it's config.
The purpose of this is to make all of the config files to be as similar as
possible for all of the members of the cluster.
Removing it from peers fixes the problem.
So there are three main roads we can follow:
1. Fix sasyncd to recognize self and handle it properly (a result)
2. It should be mentioned in manual, not to set self as a peer (an excuse)
3. We can change our internal config handling (no one else benefits)
What would You recommend as a next step ? we will do much to follow road 1,
but we might need help, eg. code review and some guidance to meet OpenBSD
needs
Furthermore if we follow road 1, is there any chance to get the reviewed,
correct, accepted fix into the tree ?
Best regards
M.K.
Good you found the problem.
As for number 1, in general it is hard to determine if an IP is your
own (or redirects to yourself), especially since network interfaces
can be added and changed dynamically. But a starup check against
getifaddrs(3) would catch the most obvious I'm my own peer cases.
A dynamic form of loop detection would be nice, it would require
adding a hostid or equivalent. That is not normally set these days and
also would change the wire format of the messages.
But even if you start work on code, a manual page change warning
against this would be a good thing.
If you post diffs to tech@ with a proper explanation they will be
picked up and reviewed, either by me or somee other developer.
BTW, I noticed there are order dependencies in sasyncd.cnf that are
not mentioned in the man page. Also the example in
src/etc/examples/sasyncd.conf does not work if you just uncomment the
lines because of those order dependencies. So there's more room for
improvement in the man page and example config.
-Otto
Correct me please if I am wrong, but in my opinion vpn gateways are usually
static setups, because of design, security, network, documentation, politics
and other reasons
So startup check should at least give a clue when something goes wrong. Of
course setting up some kind of host id would be best solution, we will get
back to it in less busy time.
The patch below makes the following changes:
1. adds startup check for peers being listed in local addresses
2. adds proper log messages in cases of peer being local address or
duplicate
3. fixes duplicate handling, which was incorrect
4. changes name of "duplicate" variable to "valid" to allow it to used in
more cases than just duplicate
Best regards
M.K.
Thanks for working on this,
Some comments:
1. I do not like lots of code in yacc rules. Better make the validation
a separate function.
2. You are comparing string representations. One is from the config
file and one is produced by inet_ntop(), that one is in canonical
form, but the first does not have to be. So you might miss equivalent
addresses. Probably better convert the ones form the config file with
inet_pton() compare those to the addresses returned by getifaddrs()
using memcmp().
3. Why use malloc for fixed size? A char buf[INET6_ADDRSTRLEN] will do,
(but see 2).
4. I would make it clear in the log message you're skipping the "self" peer.
-Otto
No problem. Thank You too :)
Ad. 1. Checking peer address moved to separate function. Three main
parts there:
1. Prepare data
2. Check for local
3. Check for duplicate
Ad. 2. Done
Ad. 3. No longer relevant
Ad. 4. Done for all messages
Best regards
M.K.
Index: conf.y
===================================================================
RCS file: /cvs/src/usr.sbin/sasyncd/conf.y,v
retrieving revision 1.19
diff -u -p -r1.19 conf.y
--- conf.y 9 Apr 2017 02:40:24 -0000 1.19
+++ conf.y 11 Mar 2019 11:04:56 -0000
@@ -30,8 +30,10 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/socket.h>
+#include <arpa/inet.h>
#include <ctype.h>
#include <fcntl.h>
+#include <ifaddrs.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -48,6 +50,7 @@ struct cfgstate cfgstate;
int conflen = 0;
char *confbuf, *confptr;
+int check_peer_addr(const char *);
int yyparse(void);
int yylex(void);
void yyerror(const char *);
@@ -172,17 +175,8 @@ setting : INTERFACE STRING
| PEER STRING
{
struct syncpeer *peer;
- int duplicate = 0;
- for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
- peer = LIST_NEXT(peer, link))
- if (strcmp($2, peer->name) == 0) {
- duplicate++;
- break;
- }
- if (duplicate)
- free($2);
- else {
+ if (check_peer_addr($2)) {
peer = calloc(1, sizeof *peer);
if (!peer) {
log_err("config: calloc(1, %lu) "
@@ -191,10 +185,11 @@ setting : INTERFACE STRING
YYERROR;
}
peer->name = $2;
- }
- LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
- cfgstate.peercnt++;
- log_msg(2, "config: add peer %s", peer->name);
+ LIST_INSERT_HEAD(&cfgstate.peerlist, peer, link);
+ cfgstate.peercnt++;
+ log_msg(2, "config: add peer %s", peer->name);
+ } else
+ free($2);
}
| LISTEN ON STRING af port
{
@@ -281,6 +276,64 @@ match(char *token)
sizeof keywords[0], match_cmp);
return k ? k->value : STRING;
+}
+
+int
+check_peer_addr(const char *peer_addr)
+{
+ int valid = 1;
+ short peer_family = AF_UNSPEC;
+ struct ifaddrs *ifap = 0, *ifa;
+ struct syncpeer *peer;
+ struct sockaddr_in sa;
+ struct sockaddr_in6 sa6;
+
+ if(inet_pton(AF_INET, peer_addr, &sa.sin_addr) == 1)
+ peer_family = AF_INET;
+
+ if(peer_family == AF_UNSPEC &&
+ inet_pton(AF_INET6, peer_addr, &sa6.sin6_addr) == 1)
+ peer_family = AF_INET6;
+
+ if(peer_family == AF_UNSPEC) {
+ log_msg(2, "config: skip unparseable peer %s", peer_addr);
+ valid = 0;
+ }
+
+ if (valid && getifaddrs(&ifap) == 0) {
+ for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
+ if (!ifa->ifa_addr || ifa->ifa_addr->sa_family != peer_family)
+ continue;
+
+ switch (ifa->ifa_addr->sa_family) {
+ case AF_INET:
+ if (memcmp(&sa.sin_addr, &((struct sockaddr_in
*)ifa->ifa_addr)->sin_addr, sizeof(struct in_addr)) == 0)
+ valid = 0;
+ break;
+ case AF_INET6:
+ if (memcmp(&sa6.sin6_addr, &((struct sockaddr_in6
*)ifa->ifa_addr)->sin6_addr, sizeof(struct in6_addr)) == 0)
+ valid = 0;
+ break;
+ }
+
+ if(!valid) {
+ log_msg(2, "config: skip local peer %s", peer_addr);
+ break;
+ }
+ }
+ freeifaddrs(ifap);
+ }
+
+ if (valid)
+ for (peer = LIST_FIRST(&cfgstate.peerlist); peer;
+ peer = LIST_NEXT(peer, link))
+ if (strcmp(peer_addr, peer->name) == 0) {
+ log_msg(2, "config: skip duplicate peer %s", peer_addr);
+ valid = 0;
+ break;
+ }
+
+ return valid;
}
int
