Winbindd using 100% of CPU. Any solution?

Jeremy Allison jra at samba.org
Tue Jan 7 11:32:41 MST 2014


On Mon, Jan 06, 2014 at 08:41:07PM -0800, Richard Sharpe wrote:
> On Mon, Jan 6, 2014 at 3:41 PM, Jeremy Allison <jra at samba.org> wrote:
> >
> > Ok, I chatted with Richard and the underlying problem
> > is a disconnect with setup_domain_child() being called on
> > domain structs that have already been added into the list,
> > instead of only being called when a struct is allocated
> > and added into the list.
> >
> > The solution to that is to move the call to setup_domain_child()
> > inside of the add_trusted_domain() function, so it is
> > only called when a new domain struct is added.
> >
> > Attached is a small patchset that does this, and
> > also keeps the logic identical by also moving the
> > setting of 'domain->primary = true' inside
> > add_trusted_domain() to ensure the current
> > ordering of variable changes is the same.
> >
> > That could also be done by adding a 'bool primary'
> > parameter to the add_trusted_domain() parameter
> > list, but I know how much Volker hates new arbitrary
> > bool parameters, so I though it was cleaner to
> > do it this way :-).
> >
> > I'm pretty sure this is the correct way to
> > fix the logic, but getting a reproducer (or
> > a site that can reproduce and test the patches)
> > will be needed.
> >
> > Please review + test. I'll also attach this
> > to the bug report (https://bugzilla.samba.org/show_bug.cgi?id=10358).
> 
> I think it looks good, but would like a comment above
> add_trusted_domain about the early return and what we do etc.
> 
> I will have a chance to test this tomorrow on the same site where the
> problem first arose.

Updated version containing the comment changes requested.

Please test + review.

Thanks,

	Jeremy.
-------------- next part --------------
From 0e097bb35b64e51f0aeda7fc4edce8832ffdfe5f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 6 Jan 2014 15:15:37 -0800
Subject: [PATCH 1/2] s3: winbindd: Move the logic of whether to set
 'domain->primary' into add_trusted_domain().

BUG: https://bugzilla.samba.org/show_bug.cgi?id=10358

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/winbindd/winbindd_util.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
index 85b014d..10ca132 100644
--- a/source3/winbindd/winbindd_util.c
+++ b/source3/winbindd/winbindd_util.c
@@ -99,6 +99,7 @@ static struct winbindd_domain *add_trusted_domain(const char *domain_name, const
 	char *idmap_config_option;
 	const char *param;
 	const char **ignored_domains, **dom;
+	int role = lp_server_role();
 
 	ignored_domains = lp_parm_string_list(-1, "winbind", "ignore domains", NULL);
 	for (dom=ignored_domains; dom && *dom; dom++) {
@@ -196,6 +197,15 @@ static struct winbindd_domain *add_trusted_domain(const char *domain_name, const
 		sid_copy(&domain->sid, sid);
 	}
 
+	/* Is this our primary domain ? */
+	if (strequal(domain_name, get_global_sam_name()) &&
+			(role != ROLE_DOMAIN_MEMBER)) {
+		domain->primary = true;
+	} else if (strequal(domain_name, lp_workgroup()) &&
+			(role == ROLE_DOMAIN_MEMBER)) {
+		domain->primary = true;
+	}
+
 	/* Link to domain list */
 	DLIST_ADD_END(_domain_list, domain, struct winbindd_domain *);
 
@@ -628,9 +638,6 @@ bool init_domain_list(void)
 	domain = add_trusted_domain(get_global_sam_name(), NULL,
 				    &cache_methods, get_global_sam_sid());
 	if (domain) {
-		if ( role != ROLE_DOMAIN_MEMBER ) {
-			domain->primary = True;
-		}
 		setup_domain_child(domain);
 	}
 
@@ -647,7 +654,6 @@ bool init_domain_list(void)
 		domain = add_trusted_domain( lp_workgroup(), lp_realm(),
 					     &cache_methods, &our_sid);
 		if (domain) {
-			domain->primary = True;
 			setup_domain_child(domain);
 
 			/* Even in the parent winbindd we'll need to
-- 
1.8.5.1


From dc8fb2812c791c60cbd523c5bae924eef162b98f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 6 Jan 2014 15:22:59 -0800
Subject: [PATCH 2/2] s3: winbindd: Move calling setup_domain_child() into
 add_trusted_domain().

Ensure it only gets called when a new domain is allocated
and added to the list.

This should fix problems with the previous logic where
setup_domain_child() was called in places where an existing
domain was returned.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=10358

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/winbindd/winbindd_util.c | 46 ++++++++++++----------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/source3/winbindd/winbindd_util.c b/source3/winbindd/winbindd_util.c
index 10ca132..a00fe14 100644
--- a/source3/winbindd/winbindd_util.c
+++ b/source3/winbindd/winbindd_util.c
@@ -89,7 +89,10 @@ static bool is_in_internal_domain(const struct dom_sid *sid)
 }
 
 
-/* Add a trusted domain to our list of domains */
+/* Add a trusted domain to our list of domains.
+   If the domain already exists in the list,
+   return it and don't re-initialize.  */
+
 static struct winbindd_domain *add_trusted_domain(const char *domain_name, const char *alt_name,
 						  struct winbindd_methods *methods,
 						  const struct dom_sid *sid)
@@ -147,7 +150,10 @@ static struct winbindd_domain *add_trusted_domain(const char *domain_name, const
 
 	if (domain != NULL) {
 		/*
-		 * We found a match. Possibly update the SID
+		 * We found a match on domain->name or
+		 * domain->alt_name. Possibly update the SID
+		 * if the stored SID was the NULL SID
+		 * and return the matching entry.
 		 */
 		if ((sid != NULL)
 		    && dom_sid_equal(&domain->sid, &global_sid_NULL)) {
@@ -242,6 +248,8 @@ static struct winbindd_domain *add_trusted_domain(const char *domain_name, const
 
 done:
 
+	setup_domain_child(domain);
+
 	DEBUG(2,("Added domain %s %s %s\n",
 		 domain->name, domain->alt_name,
 		 &domain->sid?sid_string_dbg(&domain->sid):""));
@@ -315,9 +323,7 @@ static void trustdom_list_done(struct tevent_req *req)
 	while ((p != NULL) && (*p != '\0')) {
 		char *q, *sidstr, *alt_name;
 		struct dom_sid sid;
-		struct winbindd_domain *domain;
 		char *alternate_name = NULL;
-		bool domain_exists;
 
 		alt_name = strchr(p, '\\');
 		if (alt_name == NULL) {
@@ -351,26 +357,16 @@ static void trustdom_list_done(struct tevent_req *req)
 		if ( !strequal( alt_name, "(null)" ) )
 			alternate_name = alt_name;
 
-		/* Check if we already have a child for the domain */
-		domain_exists = (find_domain_from_name_noinit(p) != NULL);
-
 		/*
 		 * We always call add_trusted_domain() cause on an existing
 		 * domain structure, it will update the SID if necessary.
 		 * This is important because we need the SID for sibling
 		 * domains.
 		 */
-		domain = add_trusted_domain(p, alternate_name,
+		(void)add_trusted_domain(p, alternate_name,
 					    &cache_methods,
 					    &sid);
 
-		/*
-		 * If the domain doesn't exist yet and got correctly added,
-		 * setup a new domain child.
-		 */
-		if (!domain_exists && domain != NULL) {
-			setup_domain_child(domain);
-		}
 		p=q;
 		if (p != NULL)
 			p += 1;
@@ -444,9 +440,6 @@ static void rescan_forest_root_trusts( void )
 						dom_list[i].dns_name,
 						&cache_methods,
 						&dom_list[i].sid );
-			if (d != NULL) {
-				setup_domain_child(d);
-			}
 		}
 
 		if (d == NULL) {
@@ -516,9 +509,6 @@ static void rescan_forest_trusts( void )
 							dom_list[i].dns_name,
 							&cache_methods,
 							&dom_list[i].sid );
-				if (d != NULL) {
-					setup_domain_child(d);
-				}
 			}
 
 			if (d == NULL) {
@@ -619,7 +609,6 @@ enum winbindd_result winbindd_dual_init_connection(struct winbindd_domain *domai
 /* Look up global info for the winbind daemon */
 bool init_domain_list(void)
 {
-	struct winbindd_domain *domain;
 	int role = lp_server_role();
 
 	/* Free existing list */
@@ -627,23 +616,18 @@ bool init_domain_list(void)
 
 	/* BUILTIN domain */
 
-	domain = add_trusted_domain("BUILTIN", NULL, &cache_methods,
+	(void)add_trusted_domain("BUILTIN", NULL, &cache_methods,
 				    &global_sid_Builtin);
-	if (domain) {
-		setup_domain_child(domain);
-	}
 
 	/* Local SAM */
 
-	domain = add_trusted_domain(get_global_sam_name(), NULL,
+	(void)add_trusted_domain(get_global_sam_name(), NULL,
 				    &cache_methods, get_global_sam_sid());
-	if (domain) {
-		setup_domain_child(domain);
-	}
 
 	/* Add ourselves as the first entry. */
 
 	if ( role == ROLE_DOMAIN_MEMBER ) {
+		struct winbindd_domain *domain;
 		struct dom_sid our_sid;
 
 		if (!secrets_fetch_domain_sid(lp_workgroup(), &our_sid)) {
@@ -654,8 +638,6 @@ bool init_domain_list(void)
 		domain = add_trusted_domain( lp_workgroup(), lp_realm(),
 					     &cache_methods, &our_sid);
 		if (domain) {
-			setup_domain_child(domain);
-
 			/* Even in the parent winbindd we'll need to
 			   talk to the DC, so try and see if we can
 			   contact it. Theoretically this isn't neccessary
-- 
1.8.5.1



More information about the samba-technical mailing list