Hi,

The attached patch fixes a locking issue that prevented SRV records from
working.

Regards,

Olivier

From 109dfc4075132881d4330f26d437dc8725a608dd Mon Sep 17 00:00:00 2001
From: Olivier Houchard <[email protected]>
Date: Mon, 6 Nov 2017 15:15:04 +0100
Subject: [PATCH] BUG/MINOR: dns: Don't try to get the server lock if it's
already held.

dns_link_resolution() can be called with the server lock already held, so
don't attempt to lock it again in that case.
---
include/proto/dns.h | 2 +-
src/dns.c | 19 ++++++++++++-------
src/server.c | 2 +-
3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/proto/dns.h b/include/proto/dns.h
index c3e3846dc..3ad79c3a4 100644
--- a/include/proto/dns.h
+++ b/include/proto/dns.h
@@ -40,7 +40,7 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p,
void **newip, short *newip_sin_family,
void *owner);

-int dns_link_resolution(void *requester, int requester_type);
+int dns_link_resolution(void *requester, int requester_type, int requester_locked);
void dns_unlink_resolution(struct dns_requester *requester);
void dns_trigger_resolution(struct dns_requester *requester);

diff --git a/src/dns.c b/src/dns.c
index 6b8746090..1d12c8421 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -550,8 +550,10 @@ static void dns_check_dns_response(struct dns_resolution *res)
char hostname[DNS_MAX_NAME_SIZE];

if (dns_dn_label_to_str(item->target, item->data_len+1,
- hostname, DNS_MAX_NAME_SIZE) == -1)
+ hostname, DNS_MAX_NAME_SIZE) == -1) {
+ SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
continue;
+ }
msg = update_server_fqdn(srv, hostname, "SRV record", 1);
if (msg)
send_log(srv->proxy, LOG_NOTICE, "%s", msg);
@@ -1307,7 +1309,7 @@ static void dns_free_resolution(struct dns_resolution *resolution)
/* Links a requester (a server or a dns_srvrq) with a resolution. It returns 0
* on success, -1 otherwise.
*/
-int dns_link_resolution(void *requester, int requester_type)
+int dns_link_resolution(void *requester, int requester_type, int requester_locked)
{
struct dns_resolution *res = NULL;
struct dns_requester *req;
@@ -1345,10 +1347,12 @@ int dns_link_resolution(void *requester, int requester_type)
goto err;

if (srv) {
- SPIN_LOCK(SERVER_LOCK, &srv->lock);
+ if (!requester_locked)
+ SPIN_LOCK(SERVER_LOCK, &srv->lock);
if (srv->dns_requester == NULL) {
if ((req = calloc(1, sizeof(*req))) == NULL) {
- SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+ if (!requester_locked)
+ SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
goto err;
}
req->owner = &srv->obj_type;
@@ -1356,7 +1360,8 @@ int dns_link_resolution(void *requester, int requester_type)
}
else
req = srv->dns_requester;
- SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
+ if (!requester_locked)
+ SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
}
else if (srvrq) {
if (srvrq->dns_requester == NULL) {
@@ -1905,14 +1910,14 @@ static int dns_finalize_config(void)

if (srv->srvrq && !srv->srvrq->resolvers) {
srv->srvrq->resolvers = srv->resolvers;
- if (dns_link_resolution(srv->srvrq, OBJ_TYPE_SRVRQ) == -1) {
+ if (dns_link_resolution(srv->srvrq, OBJ_TYPE_SRVRQ, 0) == -1) {
Alert("config : %s '%s' : unable to set DNS resolution for server '%s'.\n",
proxy_type_str(px), px->id, srv->id);
err_code |= (ERR_ALERT|ERR_ABORT);
continue;
}
}
- if (dns_link_resolution(srv, OBJ_TYPE_SERVER) == -1) {
+ if (dns_link_resolution(srv, OBJ_TYPE_SERVER, 0) == -1) {
Alert("config : %s '%s', unable to set DNS resolution for server '%s'.\n",
proxy_type_str(px), px->id, srv->id);
err_code |= (ERR_ALERT|ERR_ABORT);
diff --git a/src/server.c b/src/server.c
index ed78ca52d..adc9fd40c 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3818,7 +3818,7 @@ int srv_set_fqdn(struct server *srv, const char *hostname, int dns_locked)
if (!srv->hostname || !srv->hostname_dn)
goto err;

- if (dns_link_resolution(srv, OBJ_TYPE_SERVER) == -1)
+ if (dns_link_resolution(srv, OBJ_TYPE_SERVER, 1) == -1)
goto err;

end:
--
2.13.5

On Mon, Nov 06, 2017 at 03:19:25PM +0100, Olivier Houchard wrote:
> Hi,
>
> The attached patch fixes a locking issue that prevented SRV records from
> working.
>
> Regards,
>
> Olivier
>


And another one, that fix a deadlock that occurs when checks trigger DNs
resolutoin.

Regards,

Olivier
From 3cedd71b5338f8689004837cdcaa0ae42e48e39c Mon Sep 17 00:00:00 2001
From: Olivier Houchard <[email protected]>
Date: Mon, 6 Nov 2017 17:30:28 +0100
Subject: [PATCH] BUG/MINOR: dns: Don't lock the server lock in
snr_check_ip_callback().

snr_check_ip_callback() may be called with the server lock, so don't attempt
to lock it again, instead, make sure the callers always have the lock before
calling it.
---
src/dns.c | 6 ++++++
src/server.c | 10 ++++++----
2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/dns.c b/src/dns.c
index 1d12c8421..0f93f3ce5 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1614,7 +1614,13 @@ static void dns_resolve_recv(struct dgram_conn *dgram)
* from the cache */
tmpns = ns;
list_for_each_entry(req, &res->requesters, list) {
+ struct server *s = objt_server(req->owner);
+
+ if (s)
+ SPIN_LOCK(SERVER_LOCK, &s->lock);
req->requester_cb(req, tmpns);
+ if (s)
+ SPIN_UNLOCK(SERVER_LOCK, &s->lock);
tmpns = NULL;
}

diff --git a/src/server.c b/src/server.c
index adc9fd40c..1a78fb334 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3589,6 +3589,8 @@ int snr_update_srv_status(struct server *s, int has_no_ip)
* returns:
* 0 on error
* 1 when no error or safe ignore
+ *
+ * Must be called with server lock held
*/
int snr_resolution_cb(struct dns_requester *requester, struct dns_nameserver *nameserver)
{
@@ -3694,7 +3696,9 @@ int snr_resolution_error_cb(struct dns_requester *requester, int error_code)
s = objt_server(requester->owner);
if (!s)
return 1;
+ SPIN_LOCK(SERVER_LOCK, &s->lock);
snr_update_srv_status(s, 0);
+ SPIN_UNLOCK(SERVER_LOCK, &s->lock);
return 1;
}

@@ -3703,6 +3707,8 @@ int snr_resolution_error_cb(struct dns_requester *requester, int error_code)
* which owns <srv> and is up.
* It returns a pointer to the first server found or NULL if <ip> is not yet
* assigned.
+ *
+ * Must be called with server lock held
*/
struct server *snr_check_ip_callback(struct server *srv, void *ip, unsigned char *ip_family)
{
@@ -3712,8 +3718,6 @@ struct server *snr_check_ip_callback(struct server *srv, void *ip, unsigned char
if (!srv)
return NULL;

- SPIN_LOCK(SERVER_LOCK, &srv->lock);
-
be = srv->proxy;
for (tmpsrv = be->srv; tmpsrv; tmpsrv = tmpsrv->next) {
/* we found the current server is the same, ignore it */
@@ -3751,13 +3755,11 @@ struct server *snr_check_ip_callback(struct server *srv, void *ip, unsigned char
(tmpsrv->addr.ss_family == AF_INET6 &&
memcmp(ip, &((struct sockaddr_in6 *)&tmpsrv->addr)->sin6_addr, 16) == 0))) {
SPIN_UNLOCK(SERVER_LOCK, &tmpsrv->lock);
- SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
return tmpsrv;
}
SPIN_UNLOCK(SERVER_LOCK, &tmpsrv->lock);
}

- SPIN_UNLOCK(SERVER_LOCK, &srv->lock);

return NULL;
}
--
2.13.5

On Mon, Nov 06, 2017 at 05:40:04PM +0100, Olivier Houchard wrote:
> > The attached patch fixes a locking issue that prevented SRV records from
> > working.
> And another one, that fix a deadlock that occurs when checks trigger DNs
> resolutoin.

Both patches merged, thanks Olivier.

Willy