[PATCH] s4 standard process model: Limit number of processes forked.
Gary Lockyer
gary at catalyst.net.nz
Thu Nov 29 21:43:25 UTC 2018
Updated version with a more robust test.
Ngā mihi
Gary
On 27/11/18 22:55, Andrew Bartlett wrote:
> On Tue, 2018-11-27 at 09:29 +1300, Gary Lockyer via samba-technical
> wrote:
>> And again with the patch actually attached.
>>
>> Updated patch set fixing typo's in the WHATSNEW, and adding
>> preforkrestartdc to Samba4.pm. Thanks to Andrew for finding and fixing
>> these.
>>
>> CI results: https://gitlab.com/samba-team/devel/samba/pipelines/37865336
>
> Pushing to autobuild on sn-devel I got:
>
> [79(672)/79 at 47m38s]
> samba.tests.process_limits.python3(proclimitdc:local)
> WARNING: The "server schannel" option is deprecated
> WARNING: The "server schannel" option is deprecated
> WARNING: The "server schannel" option is deprecated
> WARNING: The "server schannel" option is deprecated
> Failed to connect to ldap URL 'ldaps://proclimitdc' - LDAP client
> internal error: NT_STATUS_CONNECTION_RESET
> Failed to connect to 'ldaps://proclimitdc' with backend 'ldaps': LDAP
> client internal error: NT_STATUS_CONNECTION_RESET
> UNEXPECTED(error):
> samba.tests.process_limits.python3.samba.tests.process_limits.StandardM
> odelProcessLimitTests.test_process_limits(proclimitdc:local)
> REASON: Exception: Exception: Traceback (most recent call last):
> File "bin/python/samba/tests/process_limits.py", line 62, in
> test_process_limits
> connections.append(self.simple_bind())
> File "bin/python/samba/tests/process_limits.py", line 48, in
> simple_bind
> credentials=creds)
> File "bin/python/samba/samdb.py", line 65, in __init__
> options=options)
> File "bin/python/samba/__init__.py", line 115, in __init__
> self.connect(url, flags, options)
> File "bin/python/samba/samdb.py", line 80, in connect
> options=options)
> _ldb.LdbError: (1, 'LDAP client internal error:
> NT_STATUS_CONNECTION_RESET')
>
> There may be a bit more work required on the tests.
>
> Sorry,
>
> Andrew Bartlett
>
-------------- next part --------------
From 8fc3de61ec8e59c7d8aa6583fa245eda7aeee5a3 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Tue, 18 Sep 2018 11:21:40 +1200
Subject: [PATCH 1/3] s4 smbd standard tests: limit forked processes
Tests to confirm the standard process model honours the smbd.conf
variable "max smbd processes", when forking a new process on accept.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
python/samba/tests/process_limits.py | 78 ++++++++++++++++++++++++++++
selftest/knownfail.d/process_limit | 1 +
selftest/target/README | 7 +++
selftest/target/Samba.pm | 1 +
selftest/target/Samba4.pm | 39 ++++++++++++++
source4/selftest/tests.py | 9 ++++
6 files changed, 135 insertions(+)
create mode 100644 python/samba/tests/process_limits.py
create mode 100644 selftest/knownfail.d/process_limit
diff --git a/python/samba/tests/process_limits.py b/python/samba/tests/process_limits.py
new file mode 100644
index 00000000000..b62906e294f
--- /dev/null
+++ b/python/samba/tests/process_limits.py
@@ -0,0 +1,78 @@
+# Tests for limiting processes forked on accept by the standard process model
+#
+# Copyright (C) Andrew Bartlett <abartlet at samba.org> 2018
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+from __future__ import print_function
+"""Tests limits on processes forked by fork on accept in the standard process
+ model.
+ NOTE: This test runs in an environment with an artificially low setting for
+ smbd max processes
+"""
+
+
+import os
+from samba.tests import TestCase
+from samba.samdb import SamDB
+from ldb import LdbError, ERR_OPERATIONS_ERROR
+
+
+class StandardModelProcessLimitTests(TestCase):
+
+ def setUp(self):
+ super(StandardModelProcessLimitTests, self).setUp()
+
+ def tearDown(self):
+ super(StandardModelProcessLimitTests, self).tearDown()
+
+ def simple_bind(self):
+ creds = self.insta_creds(template=self.get_credentials())
+ creds.set_bind_dn("%s\\%s" % (creds.get_domain(),
+ creds.get_username()))
+
+ return SamDB(url="ldaps://%s" % os.environ["SERVER"],
+ lp=self.get_loadparm(),
+ credentials=creds)
+
+ def test_process_limits(self):
+ creds = self.insta_creds(template=self.get_credentials())
+ creds.set_bind_dn("%s\\%s" % (creds.get_domain(),
+ creds.get_username()))
+
+ connections = []
+ try:
+ # Open a series of LDAP connections, the maximum number of
+ # active connections should be 20, so the 21st should fail.
+ # But as it is possible that there may be other processes holding
+ # connections, need to allow for earlier connection failures.
+ for _ in range(21):
+ connections.append(self.simple_bind())
+ self.fail(
+ "Processes not limited, able to make more than 20 connections")
+ except LdbError as e:
+ (errno, estr) = e.args
+ if errno != ERR_OPERATIONS_ERROR:
+ raise
+ if not (estr.endswith("NT_STATUS_CONNECTION_DISCONNECTED") or
+ estr.endswith("NT_STATUS_CONNECTION_RESET")):
+ raise
+ pass
+ #
+ # Clean up the connections we've just opened, by deleting the
+ # connection in python. This should invoke the talloc destructor to
+ # release any resources and close the actual connection to the server.
+ for c in connections:
+ del c
diff --git a/selftest/knownfail.d/process_limit b/selftest/knownfail.d/process_limit
new file mode 100644
index 00000000000..db34dae23ca
--- /dev/null
+++ b/selftest/knownfail.d/process_limit
@@ -0,0 +1 @@
+^samba.tests.process_limits.samba.tests.process_limits.StandardModelProcessLimitTests.test_process_limits
diff --git a/selftest/target/README b/selftest/target/README
index b25dbab97da..36b68d5dd24 100644
--- a/selftest/target/README
+++ b/selftest/target/README
@@ -108,3 +108,10 @@ preforkrestartdc testenv
Used to test killing and restarting processes under the pre-fork model. Due to
the destructive nature of the tests, it's not recommended to use this testenv
for anything else.
+
+proclimitdc testenv
+-------------------
+Used to test process limits on the standard model. It sets the number of
+allowed processes artificially low, to test that new connections are refused
+correctly. Due to the limited number of connections accepted, it's not
+recommended to use this testenv for anything else.
diff --git a/selftest/target/Samba.pm b/selftest/target/Samba.pm
index f5c490fb05b..4820f987fd7 100644
--- a/selftest/target/Samba.pm
+++ b/selftest/target/Samba.pm
@@ -425,6 +425,7 @@ sub get_interface($)
$interfaces{"offlinebackupdc"} = 44;
$interfaces{"customdc"} = 45;
$interfaces{"prockilldc"} = 46;
+ $interfaces{"proclimitdc"} = 47;
# update lib/socket_wrapper/socket_wrapper.c
# #define MAX_WRAPPED_INTERFACES 64
diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
index 0a6c85d1ba7..c54942b61fd 100755
--- a/selftest/target/Samba4.pm
+++ b/selftest/target/Samba4.pm
@@ -2228,6 +2228,7 @@ sub check_env($$)
renamedc => ["backupfromdc"],
offlinebackupdc => ["backupfromdc"],
labdc => ["backupfromdc"],
+ proclimitdc => [],
none => [],
);
@@ -2688,6 +2689,44 @@ sub setup_preforkrestartdc
return $env;
}
+#
+# ad_dc test environment used solely to test standard process model connection
+# process limits. As the limit is set artificially low it should not be used
+# for other tests.
+sub setup_proclimitdc
+{
+ my ($self, $path) = @_;
+
+ # If we didn't build with ADS, pretend this env was never available
+ if (not $self->{target3}->have_ads()) {
+ return "UNKNOWN";
+ }
+
+ my $env = $self->provision_ad_dc(
+ $path,
+ "proclimitdc",
+ "PROCLIMITDOM",
+ "proclimit.samba.example.com",
+ "max smbd processes = 20");
+ unless ($env) {
+ return undef;
+ }
+
+ $env->{NSS_WRAPPER_MODULE_SO_PATH} = undef;
+ $env->{NSS_WRAPPER_MODULE_FN_PREFIX} = undef;
+
+ if (not defined($self->check_or_start($env, "standard"))) {
+ return undef;
+ }
+
+ my $upn_array = ["$env->{REALM}.upn"];
+ my $spn_array = ["$env->{REALM}.spn"];
+
+ $self->setup_namespaces($env, $upn_array, $spn_array);
+
+ return $env;
+}
+
# Sets up a DC that's solely used to do a domain backup from. We then use the
# backupfrom-DC to create the restore-DC - this proves that the backup/restore
# process will create a Samba DC that will actually start up.
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index 972f65307d8..575c14b8be2 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -1249,3 +1249,12 @@ planoldpythontestsuite("preforkrestartdc:local",
extra_args=['-U"$USERNAME%$PASSWORD"'],
name="samba.tests.prefork_restart",
py3_compatible=True)
+planoldpythontestsuite("proclimitdc:local",
+ "samba.tests.process_limits",
+ extra_path=[
+ os.path.join(srcdir(), 'python/samba/tests')],
+ extra_args=['-U"$USERNAME%$PASSWORD"'],
+ environ={'CLIENT_IP': '127.0.0.11',
+ 'SOCKET_WRAPPER_DEFAULT_IFACE': 11},
+ name="samba.tests.process_limits",
+ py3_compatible=True)
--
2.17.1
From f4d1f3dfed0a08dd7e81e35d8f459ae109c8f204 Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Fri, 7 Sep 2018 07:04:48 +1200
Subject: [PATCH 2/3] s4 smdb standard: Limit processes forked on accept.
Limit the number of processes started by the standard model on accept.
For those services that support fork on accept, the standard model forks
a new process for each new connection. This patch limits the number of
processes to the value specified in 'max smbd processes', a value of
zero indicates that there is no limit on the number of processes that
can be forked.
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
.../smbdotconf/tuning/maxsmbdprocesses.xml | 7 ++++
selftest/knownfail.d/process_limit | 1 -
source4/smbd/process_standard.c | 33 +++++++++++++++++--
3 files changed, 38 insertions(+), 3 deletions(-)
delete mode 100644 selftest/knownfail.d/process_limit
diff --git a/docs-xml/smbdotconf/tuning/maxsmbdprocesses.xml b/docs-xml/smbdotconf/tuning/maxsmbdprocesses.xml
index a194a26a13c..700e1ad8db5 100644
--- a/docs-xml/smbdotconf/tuning/maxsmbdprocesses.xml
+++ b/docs-xml/smbdotconf/tuning/maxsmbdprocesses.xml
@@ -10,6 +10,13 @@
conditions, each user will have an <citerefentry><refentrytitle>smbd</refentrytitle>
<manvolnum>8</manvolnum></citerefentry> associated with him or her to handle connections to all
shares from a given host.</para>
+
+ <para>For a Samba ADDC running the standard process model this option
+ limits the number of processes forked to handle requests.
+ Currently new processes are only forked for ldap and netlogon
+ requests.
+ </para>
+
</description>
<value type="default">0</value>
diff --git a/selftest/knownfail.d/process_limit b/selftest/knownfail.d/process_limit
deleted file mode 100644
index db34dae23ca..00000000000
--- a/selftest/knownfail.d/process_limit
+++ /dev/null
@@ -1 +0,0 @@
-^samba.tests.process_limits.samba.tests.process_limits.StandardModelProcessLimitTests.test_process_limits
diff --git a/source4/smbd/process_standard.c b/source4/smbd/process_standard.c
index b1cc7de155d..139339c92ec 100644
--- a/source4/smbd/process_standard.c
+++ b/source4/smbd/process_standard.c
@@ -32,6 +32,9 @@
#include "lib/util/debug.h"
#include "source3/lib/messages_dgm.h"
+static unsigned connections_active = 0;
+static unsigned smbd_max_processes = 0;
+
struct standard_child_state {
const char *name;
pid_t pid;
@@ -143,8 +146,7 @@ static void standard_child_pipe_handler(struct tevent_context *ev,
if (errno == 0) {
errno = ECHILD;
}
- TALLOC_FREE(state);
- return;
+ goto done;
}
if (WIFEXITED(status)) {
status = WEXITSTATUS(status);
@@ -157,7 +159,17 @@ static void standard_child_pipe_handler(struct tevent_context *ev,
DBG_ERR("Child %d (%s) terminated with signal %d\n",
(int)state->pid, state->name, status);
}
+done:
TALLOC_FREE(state);
+ if (smbd_max_processes > 0) {
+ if (connections_active < 1) {
+ DBG_ERR("Number of active connections "
+ "less than 1 (%d)\n",
+ connections_active);
+ connections_active = 1;
+ }
+ connections_active--;
+ }
return;
}
@@ -282,6 +294,21 @@ static void standard_accept_connection(
return;
}
+ if (smbd_max_processes > 0) {
+ if (connections_active >= smbd_max_processes) {
+ DBG_ERR("(%d) connections already active, "
+ "maximum is (%d). Dropping request\n",
+ connections_active,
+ smbd_max_processes);
+ /*
+ * Drop the connection as we're overloaded at the moment
+ */
+ talloc_free(sock2);
+ return;
+ }
+ connections_active++;
+ }
+
state = setup_standard_child_pipe(ev, NULL);
if (state == NULL) {
return;
@@ -486,6 +513,8 @@ static void standard_new_task(struct tevent_context *ev,
service_details->inhibit_fork_on_accept;
proc_ctx->forked_on_accept = false;
+ smbd_max_processes = lpcfg_max_smbd_processes(lp_ctx);
+
/* setup this new task. Cluster ID is PID based for this process model */
task = new_task(ev, lp_ctx, cluster_id(pid, 0), private_data, proc_ctx);
/*
--
2.17.1
From c3daa305996cafcc4bb95094a8eb8b1cd76dbb8c Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Wed, 19 Sep 2018 15:52:15 +1200
Subject: [PATCH 3/3] WHATSNEW: standard process limits
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
WHATSNEW.txt | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/WHATSNEW.txt b/WHATSNEW.txt
index 23a88c295b5..50543637c86 100644
--- a/WHATSNEW.txt
+++ b/WHATSNEW.txt
@@ -75,6 +75,15 @@ delay between restart attempts up until it reaches "prefork maximum backoff".
Using the default sequence the restart delays (in seconds) are:
0, 10, 20, ..., 120, 120, ...
+standard process model
+----------------------
+
+When using the standard process model samba forks a new process to handle ldap
+and netlogon connections. Samba now honours the 'max smbd processes' smb.conf
+parameter. The default value of 0, indicates there is no limit. The limit
+is applied individually to netlogon and ldap. When the process limit is
+exceeded Samba drops new connections immediately.
+
REMOVED FEATURES
================
--
2.17.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181130/5923eda3/signature.sig>
More information about the samba-technical
mailing list