Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

On 04.03.2014 16:46, Michal Privoznik wrote: >On 04.03.2014 12:56, Daniel P. Berrange wrote: >>On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote: >>>https://bugzilla.redhat.com/show_bug.cgi?id=981729 >>> >>>This config tunable allows users to determine the maximum number of >>>accepted but yet not authenticated users. >>> >>>Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>--- >>> daemon/libvirtd-config.c | 1 + >>> daemon/libvirtd-config.h | 1 + >>> daemon/libvirtd.aug | 1 + >>> daemon/libvirtd.c | 1 + >>> daemon/libvirtd.conf | 4 ++++ >>> daemon/test_libvirtd.aug.in | 1 + >>> src/locking/lock_daemon.c | 3 +-- >>> src/lxc/lxc_controller.c | 2 +- >>> src/rpc/virnetserver.c | 52 >>>+++++++++++++++++++++++++++++++++++++++++++-- >>> src/rpc/virnetserver.h | 1 + >>> 10 files changed, 62 insertions(+), 5 deletions(-) >>> >>>diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c >>>index c816fda..04482c5 100644 >>>--- a/daemon/libvirtd-config.c >>>+++ b/daemon/libvirtd-config.c >>>@@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data, >>> GET_CONF_INT(conf, filename, max_workers); >>> GET_CONF_INT(conf, filename, max_clients); >>> GET_CONF_INT(conf, filename, max_queued_clients); >>>+ GET_CONF_INT(conf, filename, max_anonymous_clients); >>> >>> GET_CONF_INT(conf, filename, prio_workers); >>> >> >>You need a 'data->max_anonymous_clients = 20' initialization somewher >>in this file, otherwise it'll default to 0. > >And I think we want to leave it that way. The value of zero means the >feature is disabled. That is, libvirt won't take any actions if count of >anonymous clients exceed certain value. It will still take an action >though if the number of total clients (both auth and unauth) exceeds >max_clients. The action is stop accept()-ing new clients. Trying to >invent a bright formula to compute the correct value may lead to us >adapting to a certain use case while forgetting about other. And we've >been there before (remember 'Set reasonable RSS limit on domain >startup'?). If a specific management application using libvirt requires >these values it should set it explicitly in the config file rather than >relying on libvirt defaults (which would change as libvirt adapts to >different management application). > >What we can do is change the commented default value in config file. Is >that what you had in mind in the first place? > >> >>Also, don't we want to increase 'max_clients' to something much >>larger like 5000 now that we rely on max_anonymous_clients to >>protect against DOS attack. >> >>>diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf >>>index 073c178..880f46a 100644 >>>--- a/daemon/libvirtd.conf >>>+++ b/daemon/libvirtd.conf >>>@@ -263,6 +263,10 @@ >>> # connection succeeds. >>> #max_queued_clients = 1000 >>> >>>+# The maximum length of queue of accepted but not yet not >>>+# authenticated clients. The default value is zero, meaning >>>+# the feature is disabled. >>>+#max_anonymous_clients = 20 >> >>same not about increasing max_clients value here. >> >>>diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c >>>index e047751..054ece2 100644 >>>--- a/src/locking/lock_daemon.c >>>+++ b/src/locking/lock_daemon.c >>>@@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, >>>bool privileged) >>> } >>> >>> if (!(lockd->srv = virNetServerNew(1, 1, 0, config->max_clients, >>>- -1, 0, >>>- false, NULL, >>>+ 0, -1, 0, false, NULL, >>> virLockDaemonClientNew, >>> >>>virLockDaemonClientPreExecRestart, >>> virLockDaemonClientFree, >> >>We need to support max_anonymous_clients in the lock daemon config file >>too and increase its max clients to something huge too. Each VM started >>corresponds to an open client with the lock daemon, so we need that >>value to be quite high. I forgot to reply to this block. But my argumentation stays the same here. Moreover, there's no auth or unauth users in virlockd. I mean, all users are unauth by default and virlockd has no authentication mechanisms to make them authenticate. So either we will pass 0 here and let the rest of code deal with it as if the feature is disabled or pass config->max_clients which results in the same behavior in the end.