RE: ACL Performance (caching on object basis) (ITS#1523)

["Howard Chu" <hyc@highlandsun.com>]

> -----Original Message-----
> From: Stephan Siano [mailto:stephan.siano@suse.de]

> On Monday, 28. January 2002 01:41, Howard Chu wrote:
> > In reviewing these patches, it looks like acl_check_modlist ought to be
> > checking for any value-dependent ACLs as well, but currently isn't. Yes?
>
> Well, I wasn't sure about side effects so I left it alone. The NULL
> parameter
> means that evaluation of ACL does begins from the start, not from a value
> dependant ACL, so the only effect is a reduced write performance.
>
> However, looking into the code, it shouldn't be difficult to introduce this
> here, too, of the patch should be accepted.

I've made what I believe is the correct change to acl.c here:
@@ -1034,6 +1066,7 @@
 )
 {
        struct berval *bv;
+       ACLCache *acl_cache = NULL;

        assert( be != NULL );

@@ -1085,7 +1118,11 @@
 #endif
        }

+       slap_acl_cache_init(&acl_cache,be,op,e);
+
        for ( ; mlist != NULL; mlist = mlist->sml_next ) {
+               AccessControl *ac = NULL;
+               int ok;
                /*
                 * no-user-modification operational attributes are ignored
                 * by ACL_WRITE checking as any found here are not provided
@@ -1104,6 +1141,9 @@
                        continue;
                }

+               ok = access_allowed( be, conn, op, e, mlist->sml_desc, NULL,
+                       ACL_WRITE, &acl_cache, &ac );
+
                switch ( mlist->sml_op ) {
                case LDAP_MOD_REPLACE:
                        /*
@@ -1111,9 +1151,9 @@
                         * attribute and permission to add the specific
attribut
es.
                         * This prevents abuse from selfwriters.
                         */
-                       if ( ! access_allowed( be, conn, op, e,
-                               mlist->sml_desc, NULL, ACL_WRITE ) )
+                       if ( !ok )
                        {
+                               slap_acl_cache_destroy(acl_cache);
                                return( 0 );
                        }

@@ -1124,28 +1164,44 @@
                case LDAP_MOD_ADD:
                        assert( mlist->sml_bvalues != NULL );

+                       /* No value-specific ACLs? */
+                       if ( !ac )
+                       {
+                               /* If not attribute-level, deny */
+                               if ( !ok )
+                               {
+                                       slap_acl_cache_destroy(acl_cache);
+                                       return( 0 );
+                               }
+                               break;
+                       }
+
                        for ( bv = mlist->sml_bvalues; bv->bv_val != NULL; bv++
) {
                                if ( ! access_allowed( be, conn, op, e,
-                                       mlist->sml_desc, bv, ACL_WRITE ) )
+                                       mlist->sml_desc, bv, ACL_WRITE,
&acl_cac
he, ac ) )
                                {
+                                       slap_acl_cache_destroy(acl_cache);
                                        return( 0 );
                                }
                        }
                        break;

                case LDAP_MOD_DELETE:
-                       if ( mlist->sml_bvalues == NULL ) {
-                               if ( ! access_allowed( be, conn, op, e,
-                                       mlist->sml_desc, NULL, ACL_WRITE ) )
+                       /* No value-specific ACLs? Not value-specific? */
+                       if ( !ac || mlist->sml_bvalues == NULL ) {
+                               if ( !ok )
                                {
+                                       slap_acl_cache_destroy(acl_cache);
                                        return( 0 );
                                }
                                break;
                        }
+
                        for ( bv = mlist->sml_bvalues; bv->bv_val != NULL; bv++
) {
                                if ( ! access_allowed( be, conn, op, e,
-                                       mlist->sml_desc, bv, ACL_WRITE ) )
+                                       mlist->sml_desc, bv, ACL_WRITE,
&acl_cac
he, ac ) )
                                {
+                                       slap_acl_cache_destroy(acl_cache);
                                        return( 0 );
                                }
                        }

> > Also, since the caching is only performed on a per-entry basis, the entire
> > ACLCache structure looks unnecessary.
>
> Do you mean the whole idea is unnecessary or do you think the data structure
> is unnecessary.

The ACLCache data structure is unnecessary. It stores Backend, Operation, and
Entry, all of which remain constant for the life of the cache. The
ACLCacheEntry structure is the only thing carrying useful information.

I agree with the basic idea.

> Maybe calling that ACL cache isn't really proper, but I didn't find a better
> name. I also thought about implementing a real, longer lived ACL cache, but
> that needed to store information about the validity context of the cached
> ACLs and storing and evaluating this data seemed to me as expensive as
> evaluation the ACLs themselves.

Yes, I've reached the same conclusion as well.

> > It also seems to me that nothing is
> > gained from making the ACLCacheEntry a doubly linked list, a single link
> > would be enough since the list is only traversed in one direction.
>
> You are right about this. If the whole idea seems acceptable, I will rework
> this.
>
> Yours
> Stephan Siano

  -- Howard Chu
  Chief Architect, Symas Corp.       Director, Highland Sun
  http://www.symas.com               http://highlandsun.com/hyc
  Symas: Premier OpenSource Development and Support