0031-Issue-4925-Performance-ACI-targetfilter-evaluation-r.patch 18.2 KB
Newer Older
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
From 18a8ed29ae0b300083a6b83665b0137948a2ef7c Mon Sep 17 00:00:00 2001
From: tbordaz <tbordaz@redhat.com>
Date: Thu, 23 Sep 2021 10:48:50 +0200
Subject: [PATCH 1/3] Issue 4925 - Performance ACI: targetfilter evaluation
 result can be reused (#4926)

Bug description:
	An ACI may contain targetfilter. For a given returned entry, of a
        SRCH request, the same targetfilter is evaluated for each of the
        returned attributes.
        Once the filter has been evaluated, it is useless to reevaluate
        it for a next attribute.

Fix description:
	The fix implements a very simple cache (linked list) that keeps
        the results of the previously evaluated 'targetfilter'.
        This cache is per-entry. For an operation, a aclpb is allocated
        that is used to evaluate ACIs against each successive entry.
        Each time a candidate entry is added in the aclpb
        (acl_access_allowed), the cache (aclpb_curr_entry_targetfilters)
        is freed. Then for each 'targetfilter', the original targetfilter
        is lookup from the cache. If this is the first evaluation of it
        then the result of the evaluation is stored into the cache using
        the original targetfilter as the key in the cache

	The key to lookup/store the cache is the string representation
        of the targetfilter. The string contains a redzone to detect
        that the filter exceeds the maximum size (2K). If it exceeds
        then the key is invalid and the lookup/store is noop.

relates: #4925

Reviewed by: Mark Reynolds, William Brown (Thanks)

Platforms tested: F34
---
 ldap/servers/plugins/acl/acl.c     | 138 +++++++++++++++++++++++++++--
 ldap/servers/plugins/acl/acl.h     |  14 +++
 ldap/servers/plugins/acl/acl_ext.c |  12 +++
 ldap/servers/slapd/libglobs.c      |  29 ++++++
 ldap/servers/slapd/proto-slap.h    |   2 +
 ldap/servers/slapd/slap.h          |   2 +
 6 files changed, 191 insertions(+), 6 deletions(-)

diff --git a/ldap/servers/plugins/acl/acl.c b/ldap/servers/plugins/acl/acl.c
index 4e811f73a..377c18e68 100644
--- a/ldap/servers/plugins/acl/acl.c
+++ b/ldap/servers/plugins/acl/acl.c
@@ -67,6 +67,9 @@ static void print_access_control_summary(char *source,
                                          const char *edn,
                                          aclResultReason_t *acl_reason);
 static int check_rdn_access(Slapi_PBlock *pb, Slapi_Entry *e, const char *newrdn, int access);
+static struct targetfilter_cached_result *targetfilter_cache_lookup(struct acl_pblock *aclpb, char *filter, PRBool filter_valid);
+static void targetfilter_cache_add(struct acl_pblock *aclpb, char *filter, int result, PRBool filter_valid);
+
 
 
 /*
@@ -176,6 +179,70 @@ check_rdn_access(Slapi_PBlock *pb, Slapi_Entry *e, const char *dn, int access)
     return (retCode);
 }
 
+/* Retrieves, in the targetfilter cache (list), this
+ * filter in case it was already evaluated
+ *
+ * filter: key to retrieve the evaluation in the cache
+ * filter_valid: PR_FALSE means that the filter key is truncated, PR_TRUE else
+ */
+static struct targetfilter_cached_result *
+targetfilter_cache_lookup(struct acl_pblock *aclpb, char *filter, PRBool filter_valid)
+{
+    struct targetfilter_cached_result *results;
+    if (! aclpb->targetfilter_cache_enabled) {
+        /* targetfilter cache is disabled */
+        return NULL;
+    }
+    if (filter == NULL) {
+        return NULL;
+    }
+    for(results = aclpb->aclpb_curr_entry_targetfilters; results; results = results->next) {
+        if (strcmp(results->filter, filter) == 0) {
+            return results;
+        }
+    }
+
+    return NULL;
+}
+
+/* Free all evaluations cached for this current entry */
+void
+targetfilter_cache_free(struct acl_pblock *aclpb)
+{
+    struct targetfilter_cached_result *results, *next;
+    if (aclpb == NULL) {
+        return;
+    }
+    for(results = aclpb->aclpb_curr_entry_targetfilters; results;) {
+        next = results->next;
+        slapi_ch_free_string(&results->filter);
+        slapi_ch_free((void **) &results);
+        results = next;
+    }
+    aclpb->aclpb_curr_entry_targetfilters = NULL;
+}
+
+/* add a new targetfilter evaluation into the cache (per entry)
+ * ATM just use a linked list of evaluation
+ *
+ * filter: key to retrieve the evaluation in the cache
+ * result: result of the evaluation
+ * filter_valid: PR_FALSE means that the filter key is truncated, PR_TRUE else
+ */
+static void
+targetfilter_cache_add(struct acl_pblock *aclpb, char *filter, int result, PRBool filter_valid)
+{
+    struct targetfilter_cached_result *results;
+    if (! filter_valid || ! aclpb->targetfilter_cache_enabled) {
+        /* targetfilter cache is disabled or filter is truncated */
+        return;
+    }
+    results = (struct targetfilter_cached_result *) slapi_ch_calloc(1, (sizeof(struct targetfilter_cached_result)));
+    results->filter = slapi_ch_strdup(filter);
+    results->next = aclpb->aclpb_curr_entry_targetfilters;
+    results->matching_result = result;
+    aclpb->aclpb_curr_entry_targetfilters = results;
+}
 /***************************************************************************
 *
 * acl_access_allowed
@@ -496,6 +563,7 @@ acl_access_allowed(
 
         /* Keep the ptr to the current entry */
         aclpb->aclpb_curr_entry = (Slapi_Entry *)e;
+        targetfilter_cache_free(aclpb);
 
         /* Get the attr info */
         deallocate_attrEval = acl__get_attrEval(aclpb, attr);
@@ -1924,7 +1992,7 @@ acl_modified(Slapi_PBlock *pb, int optype, Slapi_DN *e_sdn, void *change)
 *    None.
 *
 **************************************************************************/
-static int
+int
 acl__scan_for_acis(Acl_PBlock *aclpb, int *err)
 {
     aci_t *aci;
@@ -2405,10 +2473,68 @@ acl__resource_match_aci(Acl_PBlock *aclpb, aci_t *aci, int skip_attrEval, int *a
                                                     ACL_EVAL_TARGET_FILTER);
             slapi_ch_free((void **)&lasinfo);
         } else {
-            if (slapi_vattr_filter_test(NULL, aclpb->aclpb_curr_entry,
-                                        aci->targetFilter,
-                                        0 /*don't do access check*/) != 0) {
-                filter_matched = ACL_FALSE;
+            Slapi_DN *sdn;
+            char* attr_evaluated = "None";
+            char logbuf[2048] = {0};
+            char *redzone = "the redzone";
+            int32_t redzone_idx;
+            char *filterstr; /* key to retrieve/add targetfilter value in the cache */
+            PRBool valid_filter;
+            struct targetfilter_cached_result *previous_filter_test;
+
+            /* only usefull for debug purpose */
+            if (aclpb->aclpb_curr_attrEval && aclpb->aclpb_curr_attrEval->attrEval_name) {
+                attr_evaluated = aclpb->aclpb_curr_attrEval->attrEval_name;
+            }
+            sdn = slapi_entry_get_sdn(aclpb->aclpb_curr_entry);
+
+            /* The key for the cache is the string representation of the original filter
+             * If the string can not fit into the provided buffer (overwrite redzone)
+             * then the filter is said invalid (for the cache) and it will be evaluated
+             */
+            redzone_idx = sizeof(logbuf) - 1 - strlen(redzone);
+            strcpy(&logbuf[redzone_idx], redzone);
+            filterstr = slapi_filter_to_string(aci->targetFilter, logbuf, sizeof(logbuf));
+
+            /* if the redzone was overwritten that means filterstr is truncated and not valid */
+            valid_filter = (strcmp(&logbuf[redzone_idx], redzone) == 0);
+            if (!valid_filter) {
+                strcpy(&logbuf[50], "...");
+                slapi_log_err(SLAPI_LOG_ACL, "acl__ressource_match_aci", "targetfilter too large (can not be cache) %s\n", logbuf);
+            }
+
+            previous_filter_test = targetfilter_cache_lookup(aclpb, filterstr, valid_filter);
+            if (previous_filter_test) {
+                /* The filter was already evaluated against that same entry */
+                if (previous_filter_test->matching_result == 0) {
+                    slapi_log_err(SLAPI_LOG_ACL, "acl__ressource_match_aci", "cached result for entry %s did NOT match %s (%s)\n",
+                            slapi_sdn_get_ndn(sdn),
+                            filterstr,
+                            attr_evaluated);
+                    filter_matched = ACL_FALSE;
+                } else {
+                    slapi_log_err(SLAPI_LOG_ACL, "acl__ressource_match_aci", "cached result for entry %s did match %s (%s)\n",
+                            slapi_sdn_get_ndn(sdn),
+                            filterstr,
+                            attr_evaluated);
+                }
+            } else {
+                /* The filter has not already been evaluated against that entry
+                 * evaluate it and cache the result
+                 */
+                if (slapi_vattr_filter_test(NULL, aclpb->aclpb_curr_entry,
+                        aci->targetFilter,
+                        0 /*don't do access check*/) != 0) {
+                    filter_matched = ACL_FALSE;
+                    targetfilter_cache_add(aclpb, filterstr, 0, valid_filter); /* does not match */
+                } else {
+                    targetfilter_cache_add(aclpb, filterstr, 1, valid_filter); /* does match */
+                }
+                slapi_log_err(SLAPI_LOG_ACL, "acl__ressource_match_aci", "entry %s %s match %s (%s)\n",
+                        slapi_sdn_get_ndn(sdn),
+                        filter_matched == ACL_FALSE ? "does not" : "does",
+                        filterstr,
+                        attr_evaluated);
             }
         }
 
@@ -2858,7 +2984,7 @@ acl__resource_match_aci_EXIT:
 *    None.
 *
 **************************************************************************/
-static int
+int
 acl__TestRights(Acl_PBlock *aclpb, int access, const char **right, const char **map_generic, aclResultReason_t *result_reason)
 {
     ACLEvalHandle_t *acleval;
diff --git a/ldap/servers/plugins/acl/acl.h b/ldap/servers/plugins/acl/acl.h
index becc7f920..c9b9efa56 100644
--- a/ldap/servers/plugins/acl/acl.h
+++ b/ldap/servers/plugins/acl/acl.h
@@ -407,6 +407,17 @@ struct aci_container
 };
 typedef struct aci_container AciContainer;
 
+/* This structure is stored in the aclpb.
+ * It is a linked list containing the result of
+ * the filter matching against a specific entry.
+ *
+ * This list is free for each new entry in the aclpb*/
+struct targetfilter_cached_result {
+    char *filter;                            /* strdup of string representation of aci->targetFilter */
+    int matching_result;                     /* 0 does not match / 1 does match */
+    struct targetfilter_cached_result *next; /* next targetfilter already evaluated */
+};
+
 struct acl_pblock
 {
     int aclpb_state;
@@ -476,6 +487,8 @@ struct acl_pblock
 
     /* Current entry/dn/attr evaluation info */
     Slapi_Entry *aclpb_curr_entry; /* current Entry being processed */
+    int32_t targetfilter_cache_enabled;
+    struct targetfilter_cached_result *aclpb_curr_entry_targetfilters;
     int aclpb_num_entries;
     Slapi_DN *aclpb_curr_entry_sdn;    /* Entry's SDN */
     Slapi_DN *aclpb_authorization_sdn; /* dn used for authorization */
@@ -723,6 +736,7 @@ void acl_modified(Slapi_PBlock *pb, int optype, Slapi_DN *e_sdn, void *change);
 
 int acl_access_allowed_disjoint_resource(Slapi_PBlock *pb, Slapi_Entry *e, char *attr, struct berval *val, int access);
 int acl_access_allowed_main(Slapi_PBlock *pb, Slapi_Entry *e, char **attrs, struct berval *val, int access, int flags, char **errbuf);
+void targetfilter_cache_free(struct acl_pblock *aclpb);
 int acl_access_allowed(Slapi_PBlock *pb, Slapi_Entry *e, char *attr, struct berval *val, int access);
 aclUserGroup *acl_get_usersGroup(struct acl_pblock *aclpb, char *n_dn);
 void acl_print_acllib_err(NSErr_t *errp, char *str);
diff --git a/ldap/servers/plugins/acl/acl_ext.c b/ldap/servers/plugins/acl/acl_ext.c
index 797c5d2fd..c88f7389f 100644
--- a/ldap/servers/plugins/acl/acl_ext.c
+++ b/ldap/servers/plugins/acl/acl_ext.c
@@ -189,6 +189,11 @@ acl_operation_ext_constructor(void *object __attribute__((unused)), void *parent
         slapi_log_err(SLAPI_LOG_ERR, plugin_name,
                       "acl_operation_ext_constructor - Operation extension allocation Failed\n");
     }
+    /* targetfilter_cache toggle set during aclpb allocation
+     * to avoid accessing configuration during the evaluation
+     * of each aci
+     */
+    aclpb->targetfilter_cache_enabled = config_get_targetfilter_cache();
 
     TNF_PROBE_0_DEBUG(acl_operation_ext_constructor_end, "ACL", "");
 
@@ -713,6 +718,7 @@ acl__free_aclpb(Acl_PBlock **aclpb_ptr)
     slapi_ch_free((void **)&(aclpb->aclpb_curr_entryEval_context.acle_handles_matched_target));
     slapi_ch_free((void **)&(aclpb->aclpb_prev_entryEval_context.acle_handles_matched_target));
     slapi_ch_free((void **)&(aclpb->aclpb_prev_opEval_context.acle_handles_matched_target));
+    targetfilter_cache_free(aclpb);
     slapi_sdn_free(&aclpb->aclpb_authorization_sdn);
     slapi_sdn_free(&aclpb->aclpb_curr_entry_sdn);
     if (aclpb->aclpb_macro_ht) {
@@ -921,6 +927,12 @@ acl__done_aclpb(struct acl_pblock *aclpb)
                       aclpb->aclpb_acleval ? (char *)aclpb->aclpb_acleval : "NULL");
     }
 
+    /* This aclpb return to the aclpb pool, make sure
+     * the cached evaluations are freed and that
+     * aclpb_curr_entry_targetfilters is NULL
+     */
+    targetfilter_cache_free(aclpb);
+
     /* Now Free the contents or clean it */
     slapi_sdn_done(aclpb->aclpb_curr_entry_sdn);
     if (aclpb->aclpb_Evalattr)
diff --git a/ldap/servers/slapd/libglobs.c b/ldap/servers/slapd/libglobs.c
index db7d01bbc..2ea4cd760 100644
--- a/ldap/servers/slapd/libglobs.c
+++ b/ldap/servers/slapd/libglobs.c
@@ -221,6 +221,7 @@ slapi_onoff_t init_return_exact_case;
 slapi_onoff_t init_result_tweak;
 slapi_onoff_t init_plugin_track;
 slapi_onoff_t init_moddn_aci;
+slapi_onoff_t init_targetfilter_cache;
 slapi_onoff_t init_lastmod;
 slapi_onoff_t init_readonly;
 slapi_onoff_t init_accesscontrol;
@@ -903,6 +904,11 @@ static struct config_get_and_set
      (void **)&global_slapdFrontendConfig.moddn_aci,
      CONFIG_ON_OFF, (ConfigGetFunc)config_get_moddn_aci,
      &init_moddn_aci, NULL},
+    {CONFIG_TARGETFILTER_CACHE_ATTRIBUTE, config_set_targetfilter_cache,
+     NULL, 0,
+     (void **)&global_slapdFrontendConfig.targetfilter_cache,
+     CONFIG_ON_OFF, (ConfigGetFunc)config_get_targetfilter_cache,
+     &init_targetfilter_cache, NULL},
     {CONFIG_ATTRIBUTE_NAME_EXCEPTION_ATTRIBUTE, config_set_attrname_exceptions,
      NULL, 0,
      (void **)&global_slapdFrontendConfig.attrname_exceptions,
@@ -1688,6 +1694,7 @@ FrontendConfig_init(void)
     init_syntaxcheck = cfg->syntaxcheck = LDAP_ON;
     init_plugin_track = cfg->plugin_track = LDAP_OFF;
     init_moddn_aci = cfg->moddn_aci = LDAP_ON;
+    init_targetfilter_cache = cfg->targetfilter_cache = LDAP_ON;
     init_syntaxlogging = cfg->syntaxlogging = LDAP_OFF;
     init_dn_validate_strict = cfg->dn_validate_strict = LDAP_OFF;
     init_ds4_compatible_schema = cfg->ds4_compatible_schema = LDAP_OFF;
@@ -4053,6 +4060,21 @@ config_set_moddn_aci(const char *attrname, char *value, char *errorbuf, int appl
     return retVal;
 }
 
+int32_t
+config_set_targetfilter_cache(const char *attrname, char *value, char *errorbuf, int apply)
+{
+    int32_t retVal = LDAP_SUCCESS;
+    slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
+
+    retVal = config_set_onoff(attrname,
+                              value,
+                              &(slapdFrontendConfig->targetfilter_cache),
+                              errorbuf,
+                              apply);
+
+    return retVal;
+}
+
 int32_t
 config_set_dynamic_plugins(const char *attrname, char *value, char *errorbuf, int apply)
 {
@@ -5903,6 +5925,13 @@ config_get_moddn_aci(void)
     return slapi_atomic_load_32(&(slapdFrontendConfig->moddn_aci), __ATOMIC_ACQUIRE);
 }
 
+int32_t
+config_get_targetfilter_cache(void)
+{
+    slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
+    return slapi_atomic_load_32(&(slapdFrontendConfig->targetfilter_cache), __ATOMIC_ACQUIRE);
+}
+
 int32_t
 config_get_security(void)
 {
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
index 2768d5a1d..c143f3772 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -263,6 +263,7 @@ int config_set_lastmod(const char *attrname, char *value, char *errorbuf, int ap
 int config_set_nagle(const char *attrname, char *value, char *errorbuf, int apply);
 int config_set_accesscontrol(const char *attrname, char *value, char *errorbuf, int apply);
 int config_set_moddn_aci(const char *attrname, char *value, char *errorbuf, int apply);
+int32_t config_set_targetfilter_cache(const char *attrname, char *value, char *errorbuf, int apply);
 int config_set_security(const char *attrname, char *value, char *errorbuf, int apply);
 int config_set_readonly(const char *attrname, char *value, char *errorbuf, int apply);
 int config_set_schemacheck(const char *attrname, char *value, char *errorbuf, int apply);
@@ -469,6 +470,7 @@ int config_get_accesscontrol(void);
 int config_get_return_exact_case(void);
 int config_get_result_tweak(void);
 int config_get_moddn_aci(void);
+int32_t config_get_targetfilter_cache(void);
 int config_get_security(void);
 int config_get_schemacheck(void);
 int config_get_syntaxcheck(void);
diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h
index c48516157..a3c0eff93 100644
--- a/ldap/servers/slapd/slap.h
+++ b/ldap/servers/slapd/slap.h
@@ -2229,6 +2229,7 @@ typedef struct _slapdEntryPoints
 #define CONFIG_REWRITE_RFC1274_ATTRIBUTE "nsslapd-rewrite-rfc1274"
 #define CONFIG_PLUGIN_BINDDN_TRACKING_ATTRIBUTE "nsslapd-plugin-binddn-tracking"
 #define CONFIG_MODDN_ACI_ATTRIBUTE "nsslapd-moddn-aci"
+#define CONFIG_TARGETFILTER_CACHE_ATTRIBUTE "nsslapd-targetfilter-cache"
 #define CONFIG_GLOBAL_BACKEND_LOCK "nsslapd-global-backend-lock"
 #define CONFIG_ENABLE_NUNC_STANS "nsslapd-enable-nunc-stans"
 #define CONFIG_ENABLE_UPGRADE_HASH "nsslapd-enable-upgrade-hash"
@@ -2401,6 +2402,7 @@ typedef struct _slapdFrontendConfig
     char **plugin;
     slapi_onoff_t plugin_track;
     slapi_onoff_t moddn_aci;
+    slapi_onoff_t targetfilter_cache;
     struct pw_scheme *pw_storagescheme;
     slapi_onoff_t pwpolicy_local;
     slapi_onoff_t pw_is_global_policy;
-- 
2.31.1