[systemd-devel] [PATCH 0/5] kdbus: allow multiple policies

[Kay Sievers]

On Thu, Jul 31, 2014 at 8:57 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> (Cc'ed Lennart)
>
> On Thu, Jul 31, 2014 at 05:40:53PM +0200, Kay Sievers wrote:
>> On Wed, Jul 23, 2014 at 6:34 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
>> > This series adds the infrastructure to test and upload multiple
>> > policies.
>> >
>> > The last #5 patch allows to upload multiple policies per connection
>>
>> What is the reason for this? A policy holding connection (which
>> matches a busname unit) shouldn't only be in charge of one single
>> name?
> Hmm, I thought that what you describe is for activators?

Yes, activators can only ever have one name, but policy holders should too.

> I was following the current kdbus spec/code here!
>
> From kdbus.h source:
> "@KDBUS_HELLO_POLICY_HOLDER: Special-purpose connection which registers
> policy entries for one or multiple names. The provided names are not
> activated, and are not registered with the name database"

I'm going to fix that, it does not sound correct.

We have one .busname file per name and it will get really complicated
to start stop a busname, when it has multiple names per connection. We
should really avoid that and require one connection per name and allow
only name.

> And the logic in policy.c is set that multiple policy entries can have
> the same owner which is the policy holding connection:
>
> struct kdbus_policy_db_entry {
>         char *name;
>         struct hlist_node hentry;
>         struct list_head access_list;
>         const void *owner;
>         bool wildcard:1;
>  };

Only custom endpoints will carry list of policies for different names.
It looks and acts a little bit like a firewall.

> This should allow one single connection to handle multiple policy
> entries, I think the design is good, since policy entries are then
> handled internally by kdbus, so only one single connection resources
> to achieve this, which can be reliable on private domains,buses...

Resources don't really matter here, they are cheap. We need to keep
things simple and robust, and managing lists of names in userspace
when the configuration changes would get really complicated.

> The policy holding connection acts like create policy entries, and
> invalidate them when it is closed!

Yes, all that should already work with systemd.

> Still I see three points here from how much pressure and job should
> the policy holding connection do!
> 1) Register policy entries (handled internally), no communication
> 2) Register policy entries + do basic communication based on ID
> 3) Register policy entries + acquire name or names + do communication
>    based on names...

Policy holders and activators can never communicate. Activator
connection can get messages queued, but they cannot be received by the
activator connection.

> All of these points are from the perspective: if the policy holding
> connection dies, all the registered policy entries will go away, thus
> invalidating all communications to the registered names...
>
> Should we care ?

It should all work that way already.

> And for that patch change, it will block activators from having
> multiple names, it will fail, but it will succeed for policy holders.

Which is intentional. Sorry for the misleading text in the docs.

>> > The todo for the policy holders is:
>> >
>> > * Should we set a maximum value for how many names/policies a policy
>> >  holder is allowed to upload. This is needed since we do not want to
>> >  consume all the entries (kdbus_policy_db->entries_hash).
>>
>> Yes, everything that consumes kernel memory needs to be limited.
> Ok, so this needs a proper discussion, will send about it and Cc Lennart
> too.

We need to limit the size of the uploaded policy, just like we limit
the amount of messages.

> So perhaps do not apply this series until we have a proper limit.
>
>> > * Update/test the kdbus_policy_set() to work with multiple names.
>>
>> This is meant for custom endpoints, not for policy-holder connections?
> Currently it is aimed to work for both of them!
>
> I've tested for policy-holder connections and it works with this series
> applied, but only when we create new connections. It will not work when
> we try to update or register new properties, policies or names.
>
>
> So from the source:
> connection.c:1978  it will fail, but with this patch series it will
> succeed.
>
> connection.c:1801  it will fail in kdbus_cmd_conn_update()
> Since it allows only to add/update one policy entry. So this one will be
> a todo item.
>
> And for custom endpoints, I'm sure that kdbus_policy_set() is broken, and
> it was perhaps never tested since custom endpoints are still broken, we
> can't create them, at least that was the case the last time I checked.

We never tested any of the custom endpoint stuff so far, it is just dead code.

> Kay I'm trying to have a kdbus_policy_set() version only for
> connections, so it will work for policy holding connections that handle
> multiple names! this will help to fix the sending cache issue I've been
> talking about.
>
> So please, do confirm that a policy holding connection should be able to
> register multiple policies for multiple names, before I give it more
> time. Thank you!

I confirm that we should only allow one single name. :)

> The kdbus_policy_set() for custom endpoints can be worked out later...

Yeah, we need support for it in systemd, before that we will not know
what we really need here.

Thanks,
Kay