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

[Daniel P. Berrange]

On Tue, Mar 04, 2014 at 04:56:24PM +0100, Michal Privoznik wrote:
> 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 at redhat.com>
> >>>---
> >>>  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.

I forgot we didn't do auth here. We immediately check the UID of
the client and drop them if not root, so we'd actually be perfectly
safe to increase  max_clients for the lock daemon already.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|