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@xxxxxxxxxx> > >>>--- > >>> 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list