| Message ID | 20230929195617.65120-4-jsatterfield.linux@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: avtab arrays and refactors | expand |
On Fri, Sep 29, 2023 at 3:56 PM Jacob Satterfield <jsatterfield.linux@gmail.com> wrote: > > Similar to the list_for_each macros in list.h, this patch adds two > macros that iterates an avtab_node linked list (avtab_chain_for_each and > avtab_chain_for_each_prev). This has two benefits: it reduces the amount > of duplicative code for iteration and it makes changes to the underlying > hashtable data structure easier as there are fewer places to update. You will need/want an equivalent to list_for_each_safe() or open-code it to handle avtab_destroy() below. > > Signed-off-by: Jacob Satterfield <jsatterfield.linux@gmail.com> > --- > security/selinux/ss/avtab.c | 40 ++++++++++++++++--------------------- > 1 file changed, 17 insertions(+), 23 deletions(-) > > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > index 1cd4fed30bf7..e8046eda7140 100644 > --- a/security/selinux/ss/avtab.c > +++ b/security/selinux/ss/avtab.c > @@ -223,20 +223,17 @@ avtab_search_node_next(struct avtab_node *node, u16 specified) > void avtab_destroy(struct avtab *h) > { > u32 i; > - struct avtab_node *cur, *temp; > + struct avtab_node *cur; > > if (!h) > return; > > for (i = 0; i < h->nslot; i++) { > - cur = h->htable[i]; > - while (cur) { > - temp = cur; > - cur = cur->next; > - if (temp->key.specified & AVTAB_XPERMS) > + avtab_chain_for_each(cur, h, i) { > + if (cur->key.specified & AVTAB_XPERMS) > kmem_cache_free(avtab_xperms_cachep, > - temp->datum.u.xperms); > - kmem_cache_free(avtab_node_cachep, temp); > + cur->datum.u.xperms); > + kmem_cache_free(avtab_node_cachep, cur); > } > } > kvfree(h->htable); This requires an avtab_chain_for_each_safe() or similar since it frees the node.
On Mon, Oct 2, 2023 at 10:58 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Fri, Sep 29, 2023 at 3:56 PM Jacob Satterfield > <jsatterfield.linux@gmail.com> wrote: > > > > Similar to the list_for_each macros in list.h, this patch adds two > > macros that iterates an avtab_node linked list (avtab_chain_for_each and > > avtab_chain_for_each_prev). This has two benefits: it reduces the amount > > of duplicative code for iteration and it makes changes to the underlying > > hashtable data structure easier as there are fewer places to update. > > You will need/want an equivalent to list_for_each_safe() or open-code > it to handle avtab_destroy() below. > > > > > Signed-off-by: Jacob Satterfield <jsatterfield.linux@gmail.com> > > --- > > security/selinux/ss/avtab.c | 40 ++++++++++++++++--------------------- > > 1 file changed, 17 insertions(+), 23 deletions(-) > > > > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > > index 1cd4fed30bf7..e8046eda7140 100644 > > --- a/security/selinux/ss/avtab.c > > +++ b/security/selinux/ss/avtab.c > > @@ -223,20 +223,17 @@ avtab_search_node_next(struct avtab_node *node, u16 specified) > > void avtab_destroy(struct avtab *h) > > { > > u32 i; > > - struct avtab_node *cur, *temp; > > + struct avtab_node *cur; > > > > if (!h) > > return; > > > > for (i = 0; i < h->nslot; i++) { > > - cur = h->htable[i]; > > - while (cur) { > > - temp = cur; > > - cur = cur->next; > > - if (temp->key.specified & AVTAB_XPERMS) > > + avtab_chain_for_each(cur, h, i) { > > + if (cur->key.specified & AVTAB_XPERMS) > > kmem_cache_free(avtab_xperms_cachep, > > - temp->datum.u.xperms); > > - kmem_cache_free(avtab_node_cachep, temp); > > + cur->datum.u.xperms); > > + kmem_cache_free(avtab_node_cachep, cur); > > } > > } > > kvfree(h->htable); > > This requires an avtab_chain_for_each_safe() or similar since it frees the node. Great catch! Since this separate macro would only have one invocation, it would make the code less readable without any benefit. I have reverted the changes to avtab_destroy() for the next spin. Thanks! -Jacob
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index 1cd4fed30bf7..e8046eda7140 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -27,6 +27,13 @@ static struct kmem_cache *avtab_node_cachep __ro_after_init; static struct kmem_cache *avtab_xperms_cachep __ro_after_init; +#define avtab_chain_for_each(pos, tab, slot) \ + for (pos = (tab)->htable[slot]; pos; pos = pos->next) + +#define avtab_chain_for_each_prev(pos, prev, tab, slot) \ + for (prev = NULL, pos = (tab)->htable[slot]; pos; \ + prev = pos, pos = pos->next) + /* Based on MurmurHash3, written by Austin Appleby and placed in the * public domain. */ @@ -129,9 +136,7 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key, return -EINVAL; hvalue = avtab_hash(key, h->mask); - for (prev = NULL, cur = h->htable[hvalue]; - cur; - prev = cur, cur = cur->next) { + avtab_chain_for_each_prev(cur, prev, h, hvalue) { cmp = avtab_node_cmp(key, &cur->key); /* extended perms may not be unique */ if (unlikely(cmp == 0 && !(key->specified & AVTAB_XPERMS))) @@ -163,9 +168,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, if (!h || !h->nslot || h->nel == U32_MAX) return NULL; hvalue = avtab_hash(key, h->mask); - for (prev = NULL, cur = h->htable[hvalue]; - cur; - prev = cur, cur = cur->next) { + avtab_chain_for_each_prev(cur, prev, h, hvalue) { cmp = avtab_node_cmp(key, &cur->key); if (cmp <= 0) break; @@ -180,16 +183,13 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, struct avtab_node *avtab_search_node(struct avtab *h, const struct avtab_key *key) { - u32 hvalue; struct avtab_node *cur; int cmp; if (!h || !h->nslot) return NULL; - hvalue = avtab_hash(key, h->mask); - for (cur = h->htable[hvalue]; cur; - cur = cur->next) { + avtab_chain_for_each(cur, h, avtab_hash(key, h->mask)) { cmp = avtab_node_cmp(key, &cur->key); if (cmp == 0) return cur; @@ -223,20 +223,17 @@ avtab_search_node_next(struct avtab_node *node, u16 specified) void avtab_destroy(struct avtab *h) { u32 i; - struct avtab_node *cur, *temp; + struct avtab_node *cur; if (!h) return; for (i = 0; i < h->nslot; i++) { - cur = h->htable[i]; - while (cur) { - temp = cur; - cur = cur->next; - if (temp->key.specified & AVTAB_XPERMS) + avtab_chain_for_each(cur, h, i) { + if (cur->key.specified & AVTAB_XPERMS) kmem_cache_free(avtab_xperms_cachep, - temp->datum.u.xperms); - kmem_cache_free(avtab_node_cachep, temp); + cur->datum.u.xperms); + kmem_cache_free(avtab_node_cachep, cur); } } kvfree(h->htable); @@ -307,10 +304,8 @@ void avtab_hash_eval(struct avtab *h, const char *tag) if (cur) { slots_used++; chain_len = 0; - while (cur) { + avtab_chain_for_each(cur, h, i) chain_len++; - cur = cur->next; - } if (chain_len > max_chain_len) max_chain_len = chain_len; @@ -593,8 +588,7 @@ int avtab_write(struct policydb *p, struct avtab *a, void *fp) return rc; for (i = 0; i < a->nslot; i++) { - for (cur = a->htable[i]; cur; - cur = cur->next) { + avtab_chain_for_each(cur, a, i) { rc = avtab_write_item(p, cur, fp); if (rc) return rc;
Similar to the list_for_each macros in list.h, this patch adds two macros that iterates an avtab_node linked list (avtab_chain_for_each and avtab_chain_for_each_prev). This has two benefits: it reduces the amount of duplicative code for iteration and it makes changes to the underlying hashtable data structure easier as there are fewer places to update. Signed-off-by: Jacob Satterfield <jsatterfield.linux@gmail.com> --- security/selinux/ss/avtab.c | 40 ++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 23 deletions(-)