Re: [systemd-devel] [PATCH 07/12] policy: use the db-entries_hash to access the policy db entries

2014-06-24 Thread Djalal Harouni 
Hi,

On Fri, Jun 20, 2014 at 07:12:13PM +0100, Djalal Harouni wrote:
 On Fri, Jun 20, 2014 at 08:01:04PM +0200, Daniel Mack wrote:
  On 06/20/2014 07:28 PM, Daniel Mack wrote:
   On 06/20/2014 06:50 PM, Djalal Harouni wrote:
   Use the db-entries_hash to access the policy db entries instead of the
   db-send_access_hash which is just a cache for send entries.
  
  Ah, you're purging the other entries in patch #12. Alright then, now it
  makes sense.
 Indeed, I've tested it and the cache is cleared.
 
  Applied both #7 and #9 now.
 Thank you Daniel, There is a still another series related to the cache I
 just need to test it.
 
 When we update the TALK POLICY of a connection that is already
 referenced as a *destination* in the cache we must purge all its entries
 from the cache, since permission have been changed! we need to redo the
 permission checks.
 
 This should be done in kdbus_policy_set() when we update the TALK policy
 but since kdbus_policy_set() can be called by an endpoint as an owner,
 not only a connection, I did split the code in a new function...
 
 Anyway I'll test it send it tomorrow.
Just an update:

I'm still working on the TALK cache issue, I've added the infrastracture
to clear the TALK cache (db-send_access_hash) when we update the TALK
policy of the destination connection, yes we need to clear the
previously cached entries where it is listed as destination since its
TALK permission have been changed (issue #1). However we don't want to
clear *all* the cache, only entries that apply to the newly updated
policies!

Since policy holders connections might have several policies and multiple
names, the best solution would be naturaly to clear only cached entries
related to the updated policy and name! and keep other cached entries
related to other policies and names... while working on it, I hit
another two bugs:


Issue #2: currently it seems we can't have a policy holder connection
with multiple policies and names!

This check:
https://code.google.com/p/d-bus/source/browse/connection.c#1908

Will prevent policy holders to register multiple names and policies,
it should prevent activators, since we have a one name per activator!
but not policy holders.

So I've removed this check, I've added the necessary test infrastructure
to be able to register multiple policies and names, but I'm stuck in
which I think a kdbus item alignment problem! I'll try to fix and start
with this series!


Issue #3 (not confirmed) related to issue #2 and issue #1 of course is
that in function:
__kdbus_policy_check_talk_access()
https://code.google.com/p/d-bus/source/browse/policy.c#254

That function do not receive the name of the policy to check TALK access
against! the other OWN and SEE check access functions do, but not the
'TALK' one.

The current behaviour will walk the 'conn_dst-names_list' and perform
the TALK access against all names, if one name allows the TALK then we
have access! but in that case perhaps the source connection was aiming
to send to a different name which restrcit access, not this one!

So IMHO that function and all its callers should be updated to receive
the name of the destination as a third parameter, and the 'name' should
also be saved in the TALK cache, this way we are sure that the cached
entries belong to the name being checked! So we have two issues here.

So what do you think Daniel, Kay ?

(Hope I'm not getting it wrong)

I'll try to have tests for issue #2 ASAP, and will probably handle
issue #1 since I've already added the infrastructure, and if you confirm
issue #3 I'll perhaps also work on it later too (if you don't beat me
:-)

Thank you!


-- 
Djalal Harouni
http://opendz.org
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 07/12] policy: use the db-entries_hash to access the policy db entries

2014-06-20 Thread Daniel Mack 
On 06/20/2014 06:50 PM, Djalal Harouni wrote:
 Use the db-entries_hash to access the policy db entries instead of the
 db-send_access_hash which is just a cache for send entries.
 
 Signed-off-by: Djalal Harouni tix...@opendz.org
 ---
  policy.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/policy.c b/policy.c
 index bf49f68..79d6fa4 100644
 --- a/policy.c
 +++ b/policy.c
 @@ -373,7 +373,7 @@ static void __kdbus_policy_remove_owner(struct 
 kdbus_policy_db *db,
   struct hlist_node *tmp;
   int i;
  
 - hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
 + hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)

Hmm, we need to do both, right? If an owner goes away, we need to kill
both the entries it created *and* the cached entries it is related to.

Now that I look at the code, I see that we miss an owner assignment for
the cache entries. I can fix that up later.

 @@ -482,7 +483,7 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
* At the same time, the lookup mechanism won't find any collisions
* when looking for already exising names.
*/
 - hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
 + hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)
   if (e-owner == owner) {
   struct kdbus_policy_list_entry *l;

This change looks right though.



Daniel


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 07/12] policy: use the db-entries_hash to access the policy db entries

2014-06-20 Thread Daniel Mack 
On 06/20/2014 07:28 PM, Daniel Mack wrote:
 On 06/20/2014 06:50 PM, Djalal Harouni wrote:
 Use the db-entries_hash to access the policy db entries instead of the
 db-send_access_hash which is just a cache for send entries.

Ah, you're purging the other entries in patch #12. Alright then, now it
makes sense.

Applied both #7 and #9 now.



Thanks,
Daniel



 Signed-off-by: Djalal Harouni tix...@opendz.org
 ---
  policy.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

 diff --git a/policy.c b/policy.c
 index bf49f68..79d6fa4 100644
 --- a/policy.c
 +++ b/policy.c
 @@ -373,7 +373,7 @@ static void __kdbus_policy_remove_owner(struct 
 kdbus_policy_db *db,
  struct hlist_node *tmp;
  int i;
  
 -hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
 +hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)
 
 Hmm, we need to do both, right? If an owner goes away, we need to kill
 both the entries it created *and* the cached entries it is related to.
 
 Now that I look at the code, I see that we miss an owner assignment for
 the cache entries. I can fix that up later.
 
 @@ -482,7 +483,7 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
   * At the same time, the lookup mechanism won't find any collisions
   * when looking for already exising names.
   */
 -hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
 +hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)
  if (e-owner == owner) {
  struct kdbus_policy_list_entry *l;
 
 This change looks right though.
 
 
 
 Daniel
 
 
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 07/12] policy: use the db-entries_hash to access the policy db entries

2014-06-20 Thread Djalal Harouni 
On Fri, Jun 20, 2014 at 08:01:04PM +0200, Daniel Mack wrote:
 On 06/20/2014 07:28 PM, Daniel Mack wrote:
  On 06/20/2014 06:50 PM, Djalal Harouni wrote:
  Use the db-entries_hash to access the policy db entries instead of the
  db-send_access_hash which is just a cache for send entries.
 
 Ah, you're purging the other entries in patch #12. Alright then, now it
 makes sense.
Indeed, I've tested it and the cache is cleared.

 Applied both #7 and #9 now.
Thank you Daniel, There is a still another series related to the cache I
just need to test it.

When we update the TALK POLICY of a connection that is already
referenced as a *destination* in the cache we must purge all its entries
from the cache, since permission have been changed! we need to redo the
permission checks.

This should be done in kdbus_policy_set() when we update the TALK policy
but since kdbus_policy_set() can be called by an endpoint as an owner,
not only a connection, I did split the code in a new function...

Anyway I'll test it send it tomorrow.


 
 
 Thanks,
 Daniel
 
 
 
  Signed-off-by: Djalal Harouni tix...@opendz.org
  ---
   policy.c | 7 ---
   1 file changed, 4 insertions(+), 3 deletions(-)
 
  diff --git a/policy.c b/policy.c
  index bf49f68..79d6fa4 100644
  --- a/policy.c
  +++ b/policy.c
  @@ -373,7 +373,7 @@ static void __kdbus_policy_remove_owner(struct 
  kdbus_policy_db *db,
 struct hlist_node *tmp;
 int i;
   
  -  hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
  +  hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)
  
  Hmm, we need to do both, right? If an owner goes away, we need to kill
  both the entries it created *and* the cached entries it is related to.
  
  Now that I look at the code, I see that we miss an owner assignment for
  the cache entries. I can fix that up later.
  
  @@ -482,7 +483,7 @@ int kdbus_policy_set(struct kdbus_policy_db *db,
  * At the same time, the lookup mechanism won't find any collisions
  * when looking for already exising names.
  */
  -  hash_for_each_safe(db-send_access_hash, i, tmp, e, hentry)
  +  hash_for_each_safe(db-entries_hash, i, tmp, e, hentry)
 if (e-owner == owner) {
 struct kdbus_policy_list_entry *l;
  
  This change looks right though.
  
  
  
  Daniel
  
  
  ___
  systemd-devel mailing list
  systemd-devel@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/systemd-devel
  
 

-- 
Djalal Harouni
http://opendz.org
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel