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

[Djalal Harouni]

Hi,

On Thu, Jul 31, 2014 at 10:38:47PM +0200, Kay Sievers wrote:
> 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.
Ok.

> > 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.
I do agree.

> > 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.
I see the point now!

> > 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.
Yes, I was also worried about configuration changes. Well you are right
here. I did like the fact that on a private bus, a connection might hold
multiple policies for multiple names, but yes there will be problems
when updating/deleting multiple policies...


> > 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.
Ok, I will give it a review and send patches for kdbus and the
test-kdbus-policy tests, since now in test-kdbus-policy a policy holder
is able to receive messages.

So I'll updated based on these comments, thanks!

> > 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.
Yes, but I'll give it a review and write tests for these cases.


> > 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.
Hopefully it got catched now :-)


> >> > 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. :)
Aha! :-D


> > 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.
Ok, if time permits perhaps I'll have a look later, in the mean time
I'll try to add more tests and do some review at least for the
connection stuff, so we cover more use cases.

Thanks


> Thanks,
> Kay

-- 
Djalal Harouni
http://opendz.org