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

[Michal Privoznik]

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.

>>
>>> @@ -457,6 +468,7 @@ virNetServerPtr
>>> virNetServerNewPostExecRestart(virJSONValuePtr object,
>>>       unsigned int max_workers;
>>>       unsigned int priority_workers;
>>>       unsigned int max_clients;
>>> +    unsigned int nclients_unauth_max;
>>>       unsigned int keepaliveInterval;
>>>       unsigned int keepaliveCount;
>>>       bool keepaliveRequired;
>>> @@ -482,6 +494,11 @@ virNetServerPtr
>>> virNetServerNewPostExecRestart(virJSONValuePtr object,
>>>                          _("Missing max_clients data in JSON
>>> document"));
>>>           goto error;
>>>       }
>>> +    if (virJSONValueObjectGetNumberUint(object,
>>> "nclients_unauth_max", &nclients_unauth_max) < 0) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> +                       _("Missing nclients_unauth_max data in JSON
>>> document"));
>>
>> We can't raise an error here - that'll cause failure upon RPM upgrade
>> from version which didn't have this setting. Need to set some kind of
>> sensible default value upon upgrade.
>
> The sensible default is zero in my opinion. If management application
> wants to override this, they still can set the value in the config and
> then softly reset libvirtd (by softly I mean sending it signal to reload
> the config).
>
> Michal
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list