Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 25.03.2019 o 15:08, Otto Moerbeek pisze: On Sat, Mar 23, 2019 at 06:07:02PM +0100, Michał Koc wrote: ... [snip] This is almost good. You might fold host_ip() into net_set_sa(). the double malloc and copy isn't really needed. -Otto Done, patch follows 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 - 1.19 +++ conf.y 26 Mar 2019 14:51:52 - @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -48,6 +49,7 @@ struct cfgstate cfgstate; intconflen = 0; char *confbuf, *confptr; +intcheck_peer_addr(const char *); intyyparse(void); intyylex(void); void yyerror(const char *); @@ -172,29 +174,21 @@ setting : INTERFACE STRING | PEER STRING { struct syncpeer *peer; - int duplicate = 0; - for (peer = LIST_FIRST(); 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) { + if (peer == NULL) { log_err("config: calloc(1, %lu) " "failed", sizeof *peer); free($2); YYERROR; } peer->name = $2; - } - LIST_INSERT_HEAD(, peer, link); - cfgstate.peercnt++; - log_msg(2, "config: add peer %s", peer->name); + LIST_INSERT_HEAD(, peer, link); + cfgstate.peercnt++; + log_msg(2, "config: add peer %s", peer->name); + } else + free($2); } | LISTEN ON STRING af port { @@ -281,6 +275,46 @@ match(char *token) sizeof keywords[0], match_cmp); return k ? k->value : STRING; +} + +int +check_peer_addr(const char *peer_addr) +{ + struct ifaddrs *ifap = 0, *ifa; + struct syncpeer *peer; + struct sockaddr_storage ss, peer_ss; + + if(net_set_sa((struct sockaddr *), peer_addr, 0) == -1) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + return 0; + } + + if (getifaddrs() == 0) { + for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) { + if (ifa->ifa_addr == NULL || ifa->ifa_addr->sa_family != ss.ss_family) + continue; + + if (ss.ss_len == ifa->ifa_addr->sa_len && memcmp(, ifa->ifa_addr, ss.ss_len) == 0) { + log_msg(2, "config: skip local peer %s", peer_addr); + freeifaddrs(ifap); + return 0; + } + } + freeifaddrs(ifap); + } + + for (peer = LIST_FIRST(); peer != NULL; peer = LIST_NEXT(peer, link)) { + if(net_set_sa((struct sockaddr *)_ss, peer->name, 0) == -1) { + log_msg(2, "config: net_set_sa(%s) failed", peer->name); + continue; + } + if (ss.ss_len == peer_ss.ss_len && memcmp(, _ss, ss.ss_len) == 0) { + log_msg(2, "config: skip duplicate peer %s", peer_addr); + return 0; + } + } + + return 1; } int Index: net.c === RCS file: /cvs/src/usr.sbin/sasyncd/net.c,v retrieving revision 1.23 diff -u -p -r1.23 net.c --- net.c 12 Dec 2015 20:04:23 - 1.23 +++ net.c 26 Mar 2019 14:51:52 - @@ -71,7 +71,6 @@ AES_KEY aes_key[2]; /* Local prototypes. */ static u_int8_t *net_read(struct syncpeer *, u_int32_t *, u_int32_t *); -static int net_set_sa(struct sockaddr *, char *, in_port_t); static void net_check_peers(void *); /* Pretty-print a buffer. */ @@ -752,35 +751,30 @@ net_read(struct syncpeer *p, u_int32_t * return msg; }
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 23.03.2019 o 10:09, Otto Moerbeek pisze: On Fri, Mar 22, 2019 at 09:57:29PM +0100, Michał Koc wrote: W dniu 22.03.2019 o 11:19, Michał Koc pisze: W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote: W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote: On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote: I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? See comments inline. And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _addr) == 1) + peer_family = AF_INET6; `peer_addr' must not be a CIDR network, so I'd go with the more AF agnostic getaddrinfo(3) and check for res->ai_family in any case... + if (peer_family == AF_UNSPEC) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + valid = 0; + } ..but `peer_addr' may also be a hostname, so that is not handled by your diff: net.h: char *name; /* FQDN or an IP, from conf */ getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used here. Either way, this is a job for host_ip() as found in pfctl or bgpd. Other daemons like iked still have host_v4() and host_v6(). Parsers use these functions to check for valid addresses, so I'd say sasyncd should be no exception and adopt the same approach. @@ -325,7 +386,7 @@ yylex(void) /* Numerical token? */ if (isdigit(*confptr)) { for (p = confptr; *p; p++) - if (*p == '.') /* IP address, or bad input */ + if (*p == '.' || *p == ':') /* IP address, or bad input */ This fixes the parser as in # echo peer 2001:db8::1 > conf # sasyncd -dnv -c conf config: syntax error # ./obj/sasyncd -dnv -c conf configuration OK But I have not actually tested this with a proper IPv6 setup since I do not use sasyncd; did you? @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile) if (*s == '#') { while (*s != '\n' && s < buf + conflen) s++; + while (*s == '\n' && s < buf + conflen) + s++; + s--; OK kn for this fix alone. I'll test an ipv6 seup and commit the lexer parts if those work. Michal, can you work on the first set of comments? I think Klemens is right. -Otto Of course, I'll follow the comments soon Best Regards M.K. the diff is ready, what happened here: 1. host => ip manipulation moved to host_ip() function in net.c and is using getaddrinfo() 2. net_set_sa() in net.c modified to use host_ip() Bets Regards M.K All comments applied. 1. All pointers in the patch are matched against NULL. But there are tons of pointers matched to 0 or unary operator "!" in the source code, should all of them be changed ? 2. Memory allocation repaired 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 - 1.19 +++ conf.y 23 Mar 2019 16:59:03 - @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -48,6 +50,7 @@ struct cfgstate cfgstate; intconflen = 0; char *confbuf, *confptr; +intcheck_peer_addr(const char *); intyyparse(void); intyylex(void); void yyerror(const char *); @@ -172,29 +175,21 @@ setting : INTERFACE STRING | PEER STRING { struct syncpeer *peer; - int duplicate = 0; - for (peer = LIST_FIRST(); 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) { + if (peer == NULL) { log_err("config: c
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 22.03.2019 o 11:19, Michał Koc pisze: W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote: W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote: On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote: I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? See comments inline. And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _addr) == 1) + peer_family = AF_INET6; `peer_addr' must not be a CIDR network, so I'd go with the more AF agnostic getaddrinfo(3) and check for res->ai_family in any case... + if (peer_family == AF_UNSPEC) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + valid = 0; + } ..but `peer_addr' may also be a hostname, so that is not handled by your diff: net.h: char *name; /* FQDN or an IP, from conf */ getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used here. Either way, this is a job for host_ip() as found in pfctl or bgpd. Other daemons like iked still have host_v4() and host_v6(). Parsers use these functions to check for valid addresses, so I'd say sasyncd should be no exception and adopt the same approach. @@ -325,7 +386,7 @@ yylex(void) /* Numerical token? */ if (isdigit(*confptr)) { for (p = confptr; *p; p++) - if (*p == '.') /* IP address, or bad input */ + if (*p == '.' || *p == ':') /* IP address, or bad input */ This fixes the parser as in # echo peer 2001:db8::1 > conf # sasyncd -dnv -c conf config: syntax error # ./obj/sasyncd -dnv -c conf configuration OK But I have not actually tested this with a proper IPv6 setup since I do not use sasyncd; did you? @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile) if (*s == '#') { while (*s != '\n' && s < buf + conflen) s++; + while (*s == '\n' && s < buf + conflen) + s++; + s--; OK kn for this fix alone. I'll test an ipv6 seup and commit the lexer parts if those work. Michal, can you work on the first set of comments? I think Klemens is right. -Otto Of course, I'll follow the comments soon Best Regards M.K. the diff is ready, what happened here: 1. host => ip manipulation moved to host_ip() function in net.c and is using getaddrinfo() 2. net_set_sa() in net.c modified to use host_ip() Bets Regards M.K Do not actually compare two structs sockaddr if their lengths differ. 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 - 1.19 +++ conf.y 22 Mar 2019 20:55:21 - @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -48,6 +50,7 @@ struct cfgstate cfgstate; intconflen = 0; char *confbuf, *confptr; +intcheck_peer_addr(const char *); intyyparse(void); intyylex(void); void yyerror(const char *); @@ -172,17 +175,8 @@ setting: INTERFACE STRING | PEER STRING { struct syncpeer *peer; - int duplicate = 0; - for (peer = LIST_FIRST(); 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(, peer, link); - cfgstate.peercnt++; -
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 22.03.2019 o 11:19, Michał Koc pisze: W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote: W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote: On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote: I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? See comments inline. And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _addr) == 1) + peer_family = AF_INET6; `peer_addr' must not be a CIDR network, so I'd go with the more AF agnostic getaddrinfo(3) and check for res->ai_family in any case... + if (peer_family == AF_UNSPEC) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + valid = 0; + } ..but `peer_addr' may also be a hostname, so that is not handled by your diff: net.h: char *name; /* FQDN or an IP, from conf */ getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used here. Either way, this is a job for host_ip() as found in pfctl or bgpd. Other daemons like iked still have host_v4() and host_v6(). Parsers use these functions to check for valid addresses, so I'd say sasyncd should be no exception and adopt the same approach. @@ -325,7 +386,7 @@ yylex(void) /* Numerical token? */ if (isdigit(*confptr)) { for (p = confptr; *p; p++) - if (*p == '.') /* IP address, or bad input */ + if (*p == '.' || *p == ':') /* IP address, or bad input */ This fixes the parser as in # echo peer 2001:db8::1 > conf # sasyncd -dnv -c conf config: syntax error # ./obj/sasyncd -dnv -c conf configuration OK But I have not actually tested this with a proper IPv6 setup since I do not use sasyncd; did you? @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile) if (*s == '#') { while (*s != '\n' && s < buf + conflen) s++; + while (*s == '\n' && s < buf + conflen) + s++; + s--; OK kn for this fix alone. I'll test an ipv6 seup and commit the lexer parts if those work. Michal, can you work on the first set of comments? I think Klemens is right. -Otto Of course, I'll follow the comments soon Best Regards M.K. the diff is ready, what happened here: 1. host => ip manipulation moved to host_ip() function in net.c and is using getaddrinfo() 2. net_set_sa() in net.c modified to use host_ip() Bets Regards M.K Additional range check when comparing memory contents 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 - 1.19 +++ conf.y 22 Mar 2019 14:27:49 - @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -48,6 +50,7 @@ struct cfgstate cfgstate; intconflen = 0; char *confbuf, *confptr; +intcheck_peer_addr(const char *); intyyparse(void); intyylex(void); void yyerror(const char *); @@ -172,17 +175,8 @@ setting: INTERFACE STRING | PEER STRING { struct syncpeer *peer; - int duplicate = 0; - for (peer = LIST_FIRST(); 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(, peer, link); - cfgstate.peercnt++; -
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 21.03.2019 o 11:52, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 09:28:28AM +0100, Michał Koc wrote: W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote: On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote: I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? See comments inline. And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _addr) == 1) + peer_family = AF_INET6; `peer_addr' must not be a CIDR network, so I'd go with the more AF agnostic getaddrinfo(3) and check for res->ai_family in any case... + if (peer_family == AF_UNSPEC) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + valid = 0; + } ..but `peer_addr' may also be a hostname, so that is not handled by your diff: net.h: char*name; /* FQDN or an IP, from conf */ getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used here. Either way, this is a job for host_ip() as found in pfctl or bgpd. Other daemons like iked still have host_v4() and host_v6(). Parsers use these functions to check for valid addresses, so I'd say sasyncd should be no exception and adopt the same approach. @@ -325,7 +386,7 @@ yylex(void) /* Numerical token? */ if (isdigit(*confptr)) { for (p = confptr; *p; p++) - if (*p == '.') /* IP address, or bad input */ + if (*p == '.' || *p == ':') /* IP address, or bad input */ This fixes the parser as in # echo peer 2001:db8::1 > conf # sasyncd -dnv -c conf config: syntax error # ./obj/sasyncd -dnv -c conf configuration OK But I have not actually tested this with a proper IPv6 setup since I do not use sasyncd; did you? @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile) if (*s == '#') { while (*s != '\n' && s < buf + conflen) s++; + while (*s == '\n' && s < buf + conflen) + s++; + s--; OK kn for this fix alone. I'll test an ipv6 seup and commit the lexer parts if those work. Michal, can you work on the first set of comments? I think Klemens is right. -Otto Of course, I'll follow the comments soon Best Regards M.K. the diff is ready, what happened here: 1. host => ip manipulation moved to host_ip() function in net.c and is using getaddrinfo() 2. net_set_sa() in net.c modified to use host_ip() Bets 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 - 1.19 +++ conf.y 22 Mar 2019 09:16:37 - @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -48,6 +50,7 @@ struct cfgstate cfgstate; intconflen = 0; char *confbuf, *confptr; +intcheck_peer_addr(const char *); intyyparse(void); intyylex(void); void yyerror(const char *); @@ -172,17 +175,8 @@ setting: INTERFACE STRING | PEER STRING { struct syncpeer *peer; - int duplicate = 0; - for (peer = LIST_FIRST(); 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_IN
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 21.03.2019 o 07:21, Otto Moerbeek pisze: On Thu, Mar 21, 2019 at 12:51:11AM +0100, Klemens Nanni wrote: On Tue, Mar 12, 2019 at 03:19:56PM +0100, Otto Moerbeek wrote: I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? See comments inline. And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _addr) == 1) + peer_family = AF_INET6; `peer_addr' must not be a CIDR network, so I'd go with the more AF agnostic getaddrinfo(3) and check for res->ai_family in any case... + if (peer_family == AF_UNSPEC) { + log_msg(2, "config: skip unparseable peer %s", peer_addr); + valid = 0; + } ..but `peer_addr' may also be a hostname, so that is not handled by your diff: net.h: char*name; /* FQDN or an IP, from conf */ getaddrinfo(3) can resolve however, thus inet_pton(3) should not be used here. Either way, this is a job for host_ip() as found in pfctl or bgpd. Other daemons like iked still have host_v4() and host_v6(). Parsers use these functions to check for valid addresses, so I'd say sasyncd should be no exception and adopt the same approach. @@ -325,7 +386,7 @@ yylex(void) /* Numerical token? */ if (isdigit(*confptr)) { for (p = confptr; *p; p++) - if (*p == '.') /* IP address, or bad input */ + if (*p == '.' || *p == ':') /* IP address, or bad input */ This fixes the parser as in # echo peer 2001:db8::1 > conf # sasyncd -dnv -c conf config: syntax error # ./obj/sasyncd -dnv -c conf configuration OK But I have not actually tested this with a proper IPv6 setup since I do not use sasyncd; did you? @@ -397,6 +458,9 @@ conf_parse_file(char *cfgfile) if (*s == '#') { while (*s != '\n' && s < buf + conflen) s++; + while (*s == '\n' && s < buf + conflen) + s++; + s--; OK kn for this fix alone. I'll test an ipv6 seup and commit the lexer parts if those work. Michal, can you work on the first set of comments? I think Klemens is right. -Otto Of course, I'll follow the comments soon Best Regards M.K.
Re: [patch] Re: Possible sasyncd memory leak ?
W dniu 12.03.2019 o 15:19, Otto Moerbeek pisze: On Tue, Mar 12, 2019 at 02:02:15PM +0100, Otto Moerbeek wrote: On Mon, Mar 11, 2019 at 05:11:56PM +0100, Otto Moerbeek wrote: I was going to test your diff but it does not apply. Your mailer seems to convert spaces and or tabs to funny char sequences. Please fix (test by mailing to yourself and applying with patch(1)) and resend, -Otto So I reworked the diff to apply and use the proper formatting (regulart sapces and tabs instead of UTF-8 non breaking spaces). I also fixed a case of parsing IPv6 addresses. Anyone willing to ok? And now also with a lexer bug fixed. Earlier I thougt it was an order dependency in the clauses. But is was an order dependency in comment lines and empty lines. -Otto 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 - 1.19 +++ conf.y 12 Mar 2019 14:16:23 - @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -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(); 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(, peer, link); - cfgstate.peercnt++; - log_msg(2, "config: add peer %s", peer->name); + LIST_INSERT_HEAD(, peer, link); + cfgstate.peercnt++; + log_msg(2, "config: add peer %s", peer->name); + } else + free($2); } | LISTEN ON STRING af port { @@ -284,6 +279,72 @@ match(char *token) } int +check_peer_addr(const char *peer_addr) +{ + int valid = 1; + short peer_family = AF_UNSPEC; + struct ifaddrs *ifap = NULL, *ifa; + struct syncpeer *peer; + struct sockaddr_in sa; + struct sockaddr_in6 sa6; + + if (inet_pton(AF_INET, peer_addr, _addr) == 1) + peer_family = AF_INET; + + if (peer_family == AF_UNSPEC && inet_pton(AF_INET6, peer_addr, + _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() == 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(_addr, + &((struct sockaddr_in *)ifa->ifa_addr)->sin_addr, + sizeof(struct in_addr)) == 0) + valid = 0; + break; + case AF_INET6: + if (memcmp(_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); + } + +
Re: [patch] Re: Possible sasyncd memory leak ?
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
[patch] Re: Possible sasyncd memory leak ?
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
Re: re(4)/atom freezes (was Re: [SOLVED] Re: OpenBSD 4.8 freezes on certain activities)
Hello, wait a second, I'll be able to test tomorrow, will give the result instantly best regards M.K. W dniu 2010-11-12 22:32, Stuart Henderson pisze: [moving from misc to tech@, reply-to set] On 2010-11-12, Stuart Hendersons...@spacehopper.org wrote: On 2010-11-11, Micha?? Kocm...@prime.pl wrote: Yes, it does. regards M.K. W dniu 2010-11-11 10:53, Stuart Henderson pisze: On 2010-11-10, Micha?? Kocm...@prime.pl wrote: Hi All, migrating from re to em solved the network problem With the re(4), does the system recover if you leave it for a couple of minutes? Interesting... I've seen similar symptoms on my netbook a few times recently (during p2k10) which haven't happened before, and I haven't seen it since I got back, the only difference being that I'm mostly using ral(4) at home and was using re(4) there. In my case: system just appears to freeze, no response to numlock etc, cannot enter DDB (typing blind as I'm generally in X and the machine has no serial port, but no response to ctrl-alt-esc followed by boot r). When I get time to look at it I'll try and reproduce it with GENERIC rather than GENERIC.MP (this is my main machine though and I haven't had much chance to use it to investigate this yet..) and dig through my old kernel archive.. IIRC, netblast (in the netrate package) was good at triggering this (it's the fastest packet source I know of that runs on OpenBSD; about 270Kpps UDP from an opteron 146 + bge running one instance of it). okay... at first I was having problems reproducing this, until I hit the machine with a large bunch of *incoming* packets by running netblast on a faster box. to repeat this I am doing: - boot the re(4) box, configure IP addresses, systat mbuf .1 - on a faster machine, pkg_add netrate, netblast $netbook_ip_addr 5 10 (5=packet size, 10=run for 10 seconds) the following diff reverts MCLGETI for re(4) which stops the freezes for me. unless there are serious objections or a fix, I would like to commit this tomorrow as it's better than what is in-tree. with MCLGETI and 250Kpps the freeze is almost instant; machine recovers some time (usually1min) when traffic is removed. without MCLGETI at 250Kpps it is obvious that there is starvation almost instantly (especially with the fast systat updates), but the machine keeps running normally. (at first I forgot to disable IPsec - the IPsec gateway for my netbook is an alix, so I was also able to discover that netblast is rather good at triggering the vr(4) hangs there which require ifconfig down+up to recover from - I guess I will be building a non-MCLGETI vr(4) sometime soon too). Index: re.c === RCS file: /cvs/src/sys/dev/ic/re.c,v retrieving revision 1.129 diff -u -p -r1.129 re.c --- re.c 5 Oct 2010 08:57:34 - 1.129 +++ re.c 12 Nov 2010 18:39:50 - @@ -1,4 +1,4 @@ -/* $OpenBSD: re.c,v 1.129 2010/10/05 08:57:34 mikeb Exp $ */ +/* $OpenBSD: re.c,v 1.112 2009/07/18 13:21:32 sthen Exp $ */ /* $FreeBSD: if_re.c,v 1.31 2004/09/04 07:54:05 ru Exp $ */ /* * Copyright (c) 1997, 1998-2003 @@ -162,9 +162,8 @@ static inline void re_set_bufaddr(struct int re_encap(struct rl_softc *, struct mbuf *, int *); -int re_newbuf(struct rl_softc *); +int re_newbuf(struct rl_softc *, int, struct mbuf *); int re_rx_list_init(struct rl_softc *); -void re_rx_list_fill(struct rl_softc *); int re_tx_list_init(struct rl_softc *); int re_rxeof(struct rl_softc *); int re_txeof(struct rl_softc *); @@ -1109,8 +1108,6 @@ re_attach(struct rl_softc *sc, const cha IFQ_SET_MAXLEN(ifp-if_snd, RL_TX_QLEN); IFQ_SET_READY(ifp-if_snd); - m_clsetwms(ifp, MCLBYTES, 2, RL_RX_DESC_CNT); - ifp-if_capabilities = IFCAP_VLAN_MTU | IFCAP_CSUM_IPv4 | IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; @@ -1215,18 +1212,28 @@ fail_0: int -re_newbuf(struct rl_softc *sc) +re_newbuf(struct rl_softc *sc, int idx, struct mbuf *m) { - struct mbuf *m; + struct mbuf *n = NULL; bus_dmamap_tmap; struct rl_desc *d; struct rl_rxsoft *rxs; u_int32_t cmdstat; - int error, idx; + int error; - m = MCLGETI(NULL, M_DONTWAIT,sc-sc_arpcom.ac_if, MCLBYTES); - if (!m) - return (ENOBUFS); + if (m == NULL) { + MGETHDR(n, M_DONTWAIT, MT_DATA); + if (n == NULL) + return (ENOBUFS); + + MCLGET(n, M_DONTWAIT); + if (!(n-m_flags M_EXT)) { + m_freem(n); + return (ENOBUFS); + } + m = n; + } else + m-m_data = m-m_ext.ext_buf; /* * Initialize mbuf length fields and fixup @@ -1236,15 +1243,13 @@ re_newbuf(struct rl_softc *sc) m-m_len = m-m_pkthdr.len =
Re: re(4)/atom freezes (was Re: [SOLVED] Re: OpenBSD 4.8 freezes on certain activities)
Well, it looks like the patch did the trick :) thanks a lot :) shouldn't it get to Tag OpenBSD_4_8, so everyone can benefit ? correct me if I'm wrong, but re(4) seems to be a popular interface best regards M.K. W dniu 2010-11-12 22:32, Stuart Henderson pisze: [moving from misc to tech@, reply-to set] On 2010-11-12, Stuart Hendersons...@spacehopper.org wrote: On 2010-11-11, Micha?? Kocm...@prime.pl wrote: Yes, it does. regards M.K. W dniu 2010-11-11 10:53, Stuart Henderson pisze: On 2010-11-10, Micha?? Kocm...@prime.pl wrote: Hi All, migrating from re to em solved the network problem With the re(4), does the system recover if you leave it for a couple of minutes? Interesting... I've seen similar symptoms on my netbook a few times recently (during p2k10) which haven't happened before, and I haven't seen it since I got back, the only difference being that I'm mostly using ral(4) at home and was using re(4) there. In my case: system just appears to freeze, no response to numlock etc, cannot enter DDB (typing blind as I'm generally in X and the machine has no serial port, but no response to ctrl-alt-esc followed by boot r). When I get time to look at it I'll try and reproduce it with GENERIC rather than GENERIC.MP (this is my main machine though and I haven't had much chance to use it to investigate this yet..) and dig through my old kernel archive.. IIRC, netblast (in the netrate package) was good at triggering this (it's the fastest packet source I know of that runs on OpenBSD; about 270Kpps UDP from an opteron 146 + bge running one instance of it). okay... at first I was having problems reproducing this, until I hit the machine with a large bunch of *incoming* packets by running netblast on a faster box. to repeat this I am doing: - boot the re(4) box, configure IP addresses, systat mbuf .1 - on a faster machine, pkg_add netrate, netblast $netbook_ip_addr 5 10 (5=packet size, 10=run for 10 seconds) the following diff reverts MCLGETI for re(4) which stops the freezes for me. unless there are serious objections or a fix, I would like to commit this tomorrow as it's better than what is in-tree. with MCLGETI and 250Kpps the freeze is almost instant; machine recovers some time (usually1min) when traffic is removed. without MCLGETI at 250Kpps it is obvious that there is starvation almost instantly (especially with the fast systat updates), but the machine keeps running normally. (at first I forgot to disable IPsec - the IPsec gateway for my netbook is an alix, so I was also able to discover that netblast is rather good at triggering the vr(4) hangs there which require ifconfig down+up to recover from - I guess I will be building a non-MCLGETI vr(4) sometime soon too). Index: re.c === RCS file: /cvs/src/sys/dev/ic/re.c,v retrieving revision 1.129 diff -u -p -r1.129 re.c --- re.c5 Oct 2010 08:57:34 - 1.129 +++ re.c12 Nov 2010 18:39:50 - @@ -1,4 +1,4 @@ -/* $OpenBSD: re.c,v 1.129 2010/10/05 08:57:34 mikeb Exp $ */ +/* $OpenBSD: re.c,v 1.112 2009/07/18 13:21:32 sthen Exp $ */ /*$FreeBSD: if_re.c,v 1.31 2004/09/04 07:54:05 ru Exp $ */ /* * Copyright (c) 1997, 1998-2003 @@ -162,9 +162,8 @@ static inline void re_set_bufaddr(struct int re_encap(struct rl_softc *, struct mbuf *, int *); -intre_newbuf(struct rl_softc *); +intre_newbuf(struct rl_softc *, int, struct mbuf *); int re_rx_list_init(struct rl_softc *); -void re_rx_list_fill(struct rl_softc *); int re_tx_list_init(struct rl_softc *); int re_rxeof(struct rl_softc *); int re_txeof(struct rl_softc *); @@ -1109,8 +1108,6 @@ re_attach(struct rl_softc *sc, const cha IFQ_SET_MAXLEN(ifp-if_snd, RL_TX_QLEN); IFQ_SET_READY(ifp-if_snd); - m_clsetwms(ifp, MCLBYTES, 2, RL_RX_DESC_CNT); - ifp-if_capabilities = IFCAP_VLAN_MTU | IFCAP_CSUM_IPv4 | IFCAP_CSUM_TCPv4 | IFCAP_CSUM_UDPv4; @@ -1215,18 +1212,28 @@ fail_0: int -re_newbuf(struct rl_softc *sc) +re_newbuf(struct rl_softc *sc, int idx, struct mbuf *m) { - struct mbuf *m; + struct mbuf *n = NULL; bus_dmamap_tmap; struct rl_desc *d; struct rl_rxsoft *rxs; u_int32_t cmdstat; - int error, idx; + int error; - m = MCLGETI(NULL, M_DONTWAIT,sc-sc_arpcom.ac_if, MCLBYTES); - if (!m) - return (ENOBUFS); + if (m == NULL) { + MGETHDR(n, M_DONTWAIT, MT_DATA); + if (n == NULL) + return (ENOBUFS); + + MCLGET(n, M_DONTWAIT); + if (!(n-m_flags M_EXT)) { + m_freem(n); + return (ENOBUFS); + } + m = n; + } else + m-m_data = m-m_ext.ext_buf; /* * Initialize mbuf length fields and