| Message ID | 20231018175744.39667-2-jsatterfield.linux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: avtab arrays and refactors | expand |
On Wed, Oct 18, 2023 at 1:58 PM Jacob Satterfield <jsatterfield.linux@gmail.com> wrote: > > In four separate functions within avtab, the same comparison logic is > used. The only difference is how the result is handled or whether there > is a unique specifier value to be checked for or used. > > Extracting this functionality into the avtab_node_cmp() function unifies > the comparison logic between searching and insertion and gets rid of > duplicative code so that the implementation is easier to maintain. > > Signed-off-by: Jacob Satterfield <jsatterfield.linux@gmail.com> > Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com> Nit: You can't retain a Reviewed-by line if you make any changes to the previously reviewed patch. That said, having re-reviewed this one, you can add my Reviewed-by tag to your next one if there are no changes. > --- > security/selinux/ss/avtab.c | 101 +++++++++++++++--------------------- > 1 file changed, 41 insertions(+), 60 deletions(-) > > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > index 8751a602ead2..697eb4352439 100644 > --- a/security/selinux/ss/avtab.c > +++ b/security/selinux/ss/avtab.c > @@ -96,12 +96,34 @@ avtab_insert_node(struct avtab *h, struct avtab_node **dst, > return newnode; > } > > +static int avtab_node_cmp(const struct avtab_key *key1, > + const struct avtab_key *key2) > +{ > + u16 specified = key1->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > + > + if (key1->source_type == key2->source_type && > + key1->target_type == key2->target_type && > + key1->target_class == key2->target_class && > + (specified & key2->specified)) > + return 0; > + if (key1->source_type < key2->source_type) > + return -1; > + if (key1->source_type == key2->source_type && > + key1->target_type < key2->target_type) > + return -1; > + if (key1->source_type == key2->source_type && > + key1->target_type == key2->target_type && > + key1->target_class < key2->target_class) > + return -1; > + return 1; > +} > + > static int avtab_insert(struct avtab *h, const struct avtab_key *key, > const struct avtab_datum *datum) > { > u32 hvalue; > struct avtab_node *prev, *cur, *newnode; > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > + int cmp; > > if (!h || !h->nslot || h->nel == U32_MAX) > return -EINVAL; > @@ -110,23 +132,11 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key, > for (prev = NULL, cur = h->htable[hvalue]; > cur; > prev = cur, cur = cur->next) { > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class == cur->key.target_class && > - (specified & cur->key.specified)) { > - /* extended perms may not be unique */ > - if (specified & AVTAB_XPERMS) > - break; > + cmp = avtab_node_cmp(key, &cur->key); > + /* extended perms may not be unique */ > + if (cmp == 0 && !(key->specified & AVTAB_XPERMS)) > return -EEXIST; > - } > - if (key->source_type < cur->key.source_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type < cur->key.target_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class < cur->key.target_class) > + if (cmp <= 0) > break; > } > > @@ -148,7 +158,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, > { > u32 hvalue; > struct avtab_node *prev, *cur; > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > + int cmp; > > if (!h || !h->nslot || h->nel == U32_MAX) > return NULL; > @@ -156,19 +166,8 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, > for (prev = NULL, cur = h->htable[hvalue]; > cur; > prev = cur, cur = cur->next) { > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class == cur->key.target_class && > - (specified & cur->key.specified)) > - break; > - if (key->source_type < cur->key.source_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type < cur->key.target_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class < cur->key.target_class) > + cmp = avtab_node_cmp(key, &cur->key); > + if (cmp <= 0) > break; > } > return avtab_insert_node(h, prev ? &prev->next : &h->htable[hvalue], > @@ -183,7 +182,7 @@ struct avtab_node *avtab_search_node(struct avtab *h, > { > u32 hvalue; > struct avtab_node *cur; > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > + int cmp; > > if (!h || !h->nslot) > return NULL; > @@ -191,20 +190,10 @@ struct avtab_node *avtab_search_node(struct avtab *h, > hvalue = avtab_hash(key, h->mask); > for (cur = h->htable[hvalue]; cur; > cur = cur->next) { > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class == cur->key.target_class && > - (specified & cur->key.specified)) > + cmp = avtab_node_cmp(key, &cur->key); > + if (cmp == 0) > return cur; > - > - if (key->source_type < cur->key.source_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type < cur->key.target_type) > - break; > - if (key->source_type == cur->key.source_type && > - key->target_type == cur->key.target_type && > - key->target_class < cur->key.target_class) > + if (cmp < 0) > break; > } > return NULL; > @@ -213,27 +202,19 @@ struct avtab_node *avtab_search_node(struct avtab *h, > struct avtab_node* > avtab_search_node_next(struct avtab_node *node, u16 specified) > { > + struct avtab_key tmp_key; > struct avtab_node *cur; > + int cmp; > > if (!node) > return NULL; > - > - specified &= ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > + tmp_key = node->key; > + tmp_key.specified = specified; > for (cur = node->next; cur; cur = cur->next) { > - if (node->key.source_type == cur->key.source_type && > - node->key.target_type == cur->key.target_type && > - node->key.target_class == cur->key.target_class && > - (specified & cur->key.specified)) > + cmp = avtab_node_cmp(&tmp_key, &cur->key); > + if (cmp == 0) > return cur; > - > - if (node->key.source_type < cur->key.source_type) > - break; > - if (node->key.source_type == cur->key.source_type && > - node->key.target_type < cur->key.target_type) > - break; > - if (node->key.source_type == cur->key.source_type && > - node->key.target_type == cur->key.target_type && > - node->key.target_class < cur->key.target_class) > + if (cmp < 0) > break; > } > return NULL; > -- > 2.41.0 >
On Fri, Oct 20, 2023 at 9:52 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Wed, Oct 18, 2023 at 1:58 PM Jacob Satterfield > <jsatterfield.linux@gmail.com> wrote: > > > > In four separate functions within avtab, the same comparison logic is > > used. The only difference is how the result is handled or whether there > > is a unique specifier value to be checked for or used. > > > > Extracting this functionality into the avtab_node_cmp() function unifies > > the comparison logic between searching and insertion and gets rid of > > duplicative code so that the implementation is easier to maintain. > > > > Signed-off-by: Jacob Satterfield <jsatterfield.linux@gmail.com> > > Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > Nit: You can't retain a Reviewed-by line if you make any changes to > the previously reviewed patch. > That said, having re-reviewed this one, you can add my Reviewed-by tag > to your next one if there are no changes. > My apologies. Understood for the future. Thanks for the re-review. > > --- > > security/selinux/ss/avtab.c | 101 +++++++++++++++--------------------- > > 1 file changed, 41 insertions(+), 60 deletions(-) > > > > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > > index 8751a602ead2..697eb4352439 100644 > > --- a/security/selinux/ss/avtab.c > > +++ b/security/selinux/ss/avtab.c > > @@ -96,12 +96,34 @@ avtab_insert_node(struct avtab *h, struct avtab_node **dst, > > return newnode; > > } > > > > +static int avtab_node_cmp(const struct avtab_key *key1, > > + const struct avtab_key *key2) > > +{ > > + u16 specified = key1->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > > + > > + if (key1->source_type == key2->source_type && > > + key1->target_type == key2->target_type && > > + key1->target_class == key2->target_class && > > + (specified & key2->specified)) > > + return 0; > > + if (key1->source_type < key2->source_type) > > + return -1; > > + if (key1->source_type == key2->source_type && > > + key1->target_type < key2->target_type) > > + return -1; > > + if (key1->source_type == key2->source_type && > > + key1->target_type == key2->target_type && > > + key1->target_class < key2->target_class) > > + return -1; > > + return 1; > > +} > > + > > static int avtab_insert(struct avtab *h, const struct avtab_key *key, > > const struct avtab_datum *datum) > > { > > u32 hvalue; > > struct avtab_node *prev, *cur, *newnode; > > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > > + int cmp; > > > > if (!h || !h->nslot || h->nel == U32_MAX) > > return -EINVAL; > > @@ -110,23 +132,11 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key, > > for (prev = NULL, cur = h->htable[hvalue]; > > cur; > > prev = cur, cur = cur->next) { > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class == cur->key.target_class && > > - (specified & cur->key.specified)) { > > - /* extended perms may not be unique */ > > - if (specified & AVTAB_XPERMS) > > - break; > > + cmp = avtab_node_cmp(key, &cur->key); > > + /* extended perms may not be unique */ > > + if (cmp == 0 && !(key->specified & AVTAB_XPERMS)) > > return -EEXIST; > > - } > > - if (key->source_type < cur->key.source_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type < cur->key.target_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class < cur->key.target_class) > > + if (cmp <= 0) > > break; > > } > > > > @@ -148,7 +158,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, > > { > > u32 hvalue; > > struct avtab_node *prev, *cur; > > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > > + int cmp; > > > > if (!h || !h->nslot || h->nel == U32_MAX) > > return NULL; > > @@ -156,19 +166,8 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, > > for (prev = NULL, cur = h->htable[hvalue]; > > cur; > > prev = cur, cur = cur->next) { > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class == cur->key.target_class && > > - (specified & cur->key.specified)) > > - break; > > - if (key->source_type < cur->key.source_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type < cur->key.target_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class < cur->key.target_class) > > + cmp = avtab_node_cmp(key, &cur->key); > > + if (cmp <= 0) > > break; > > } > > return avtab_insert_node(h, prev ? &prev->next : &h->htable[hvalue], > > @@ -183,7 +182,7 @@ struct avtab_node *avtab_search_node(struct avtab *h, > > { > > u32 hvalue; > > struct avtab_node *cur; > > - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > > + int cmp; > > > > if (!h || !h->nslot) > > return NULL; > > @@ -191,20 +190,10 @@ struct avtab_node *avtab_search_node(struct avtab *h, > > hvalue = avtab_hash(key, h->mask); > > for (cur = h->htable[hvalue]; cur; > > cur = cur->next) { > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class == cur->key.target_class && > > - (specified & cur->key.specified)) > > + cmp = avtab_node_cmp(key, &cur->key); > > + if (cmp == 0) > > return cur; > > - > > - if (key->source_type < cur->key.source_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type < cur->key.target_type) > > - break; > > - if (key->source_type == cur->key.source_type && > > - key->target_type == cur->key.target_type && > > - key->target_class < cur->key.target_class) > > + if (cmp < 0) > > break; > > } > > return NULL; > > @@ -213,27 +202,19 @@ struct avtab_node *avtab_search_node(struct avtab *h, > > struct avtab_node* > > avtab_search_node_next(struct avtab_node *node, u16 specified) > > { > > + struct avtab_key tmp_key; > > struct avtab_node *cur; > > + int cmp; > > > > if (!node) > > return NULL; > > - > > - specified &= ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); > > + tmp_key = node->key; > > + tmp_key.specified = specified; > > for (cur = node->next; cur; cur = cur->next) { > > - if (node->key.source_type == cur->key.source_type && > > - node->key.target_type == cur->key.target_type && > > - node->key.target_class == cur->key.target_class && > > - (specified & cur->key.specified)) > > + cmp = avtab_node_cmp(&tmp_key, &cur->key); > > + if (cmp == 0) > > return cur; > > - > > - if (node->key.source_type < cur->key.source_type) > > - break; > > - if (node->key.source_type == cur->key.source_type && > > - node->key.target_type < cur->key.target_type) > > - break; > > - if (node->key.source_type == cur->key.source_type && > > - node->key.target_type == cur->key.target_type && > > - node->key.target_class < cur->key.target_class) > > + if (cmp < 0) > > break; > > } > > return NULL; > > -- > > 2.41.0 > >
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index 8751a602ead2..697eb4352439 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -96,12 +96,34 @@ avtab_insert_node(struct avtab *h, struct avtab_node **dst, return newnode; } +static int avtab_node_cmp(const struct avtab_key *key1, + const struct avtab_key *key2) +{ + u16 specified = key1->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); + + if (key1->source_type == key2->source_type && + key1->target_type == key2->target_type && + key1->target_class == key2->target_class && + (specified & key2->specified)) + return 0; + if (key1->source_type < key2->source_type) + return -1; + if (key1->source_type == key2->source_type && + key1->target_type < key2->target_type) + return -1; + if (key1->source_type == key2->source_type && + key1->target_type == key2->target_type && + key1->target_class < key2->target_class) + return -1; + return 1; +} + static int avtab_insert(struct avtab *h, const struct avtab_key *key, const struct avtab_datum *datum) { u32 hvalue; struct avtab_node *prev, *cur, *newnode; - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); + int cmp; if (!h || !h->nslot || h->nel == U32_MAX) return -EINVAL; @@ -110,23 +132,11 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key, for (prev = NULL, cur = h->htable[hvalue]; cur; prev = cur, cur = cur->next) { - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class == cur->key.target_class && - (specified & cur->key.specified)) { - /* extended perms may not be unique */ - if (specified & AVTAB_XPERMS) - break; + cmp = avtab_node_cmp(key, &cur->key); + /* extended perms may not be unique */ + if (cmp == 0 && !(key->specified & AVTAB_XPERMS)) return -EEXIST; - } - if (key->source_type < cur->key.source_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type < cur->key.target_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class < cur->key.target_class) + if (cmp <= 0) break; } @@ -148,7 +158,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, { u32 hvalue; struct avtab_node *prev, *cur; - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); + int cmp; if (!h || !h->nslot || h->nel == U32_MAX) return NULL; @@ -156,19 +166,8 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h, for (prev = NULL, cur = h->htable[hvalue]; cur; prev = cur, cur = cur->next) { - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class == cur->key.target_class && - (specified & cur->key.specified)) - break; - if (key->source_type < cur->key.source_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type < cur->key.target_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class < cur->key.target_class) + cmp = avtab_node_cmp(key, &cur->key); + if (cmp <= 0) break; } return avtab_insert_node(h, prev ? &prev->next : &h->htable[hvalue], @@ -183,7 +182,7 @@ struct avtab_node *avtab_search_node(struct avtab *h, { u32 hvalue; struct avtab_node *cur; - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); + int cmp; if (!h || !h->nslot) return NULL; @@ -191,20 +190,10 @@ struct avtab_node *avtab_search_node(struct avtab *h, hvalue = avtab_hash(key, h->mask); for (cur = h->htable[hvalue]; cur; cur = cur->next) { - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class == cur->key.target_class && - (specified & cur->key.specified)) + cmp = avtab_node_cmp(key, &cur->key); + if (cmp == 0) return cur; - - if (key->source_type < cur->key.source_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type < cur->key.target_type) - break; - if (key->source_type == cur->key.source_type && - key->target_type == cur->key.target_type && - key->target_class < cur->key.target_class) + if (cmp < 0) break; } return NULL; @@ -213,27 +202,19 @@ struct avtab_node *avtab_search_node(struct avtab *h, struct avtab_node* avtab_search_node_next(struct avtab_node *node, u16 specified) { + struct avtab_key tmp_key; struct avtab_node *cur; + int cmp; if (!node) return NULL; - - specified &= ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD); + tmp_key = node->key; + tmp_key.specified = specified; for (cur = node->next; cur; cur = cur->next) { - if (node->key.source_type == cur->key.source_type && - node->key.target_type == cur->key.target_type && - node->key.target_class == cur->key.target_class && - (specified & cur->key.specified)) + cmp = avtab_node_cmp(&tmp_key, &cur->key); + if (cmp == 0) return cur; - - if (node->key.source_type < cur->key.source_type) - break; - if (node->key.source_type == cur->key.source_type && - node->key.target_type < cur->key.target_type) - break; - if (node->key.source_type == cur->key.source_type && - node->key.target_type == cur->key.target_type && - node->key.target_class < cur->key.target_class) + if (cmp < 0) break; } return NULL;