On Fri, Oct 20, 2023 at 9:52 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Wed, Oct 18, 2023 at 1:58 PM Jacob Satterfield
> <jsatterfield.linux@xxxxxxxxx> 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@xxxxxxxxx>
> > Reviewed-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
>
> 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
> >
