On 10/21/2013 07:32 PM, Eric Paris wrote:
> From: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> Date: Tue, 18 Dec 2012 19:37:46 +0000
>
> Update the policy version (POLICYDB_VERSION_CONSTRAINT_NAMES) to allow
> holding of policy source info for constraints.
>
> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> ---
> security/selinux/include/security.h | 3 +-
> security/selinux/ss/constraint.h | 1 +
> security/selinux/ss/policydb.c | 100 ++++++++++++++++++++++++++++++++---
> security/selinux/ss/policydb.h | 11 ++++
> 4 files changed, 107 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h
> index 149dda7..3855ea8 100644
> --- a/security/selinux/ss/constraint.h
> +++ b/security/selinux/ss/constraint.h
> @@ -48,6 +48,7 @@ struct constraint_expr {
> u32 op; /* operator */
>
> struct ebitmap names; /* names */
> + struct type_set_datum *type_names;
Why did you change the name of the structure in the kernel from the one
used in libsepol (struct type_set)?
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 9cd9b7c..f1b23df 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -613,11 +618,20 @@ static int common_destroy(void *key, void *datum, void *p)
> return 0;
> }
>
> -static int cls_destroy(void *key, void *datum, void *p)
> +static void type_set_destroy(struct type_set_datum * t)
> +{
> + if (t != NULL) {
> + ebitmap_destroy(&t->types);
> + ebitmap_destroy(&t->negset);
> + }
> +}
So you don't free(t) here, yet every caller does so afterward.
Although I see it isn't that way in libsepol so maybe you are trying to
stay consistent in case you use these elsewhere in the kernel as is done
in libsepol?
> @@ -629,6 +643,10 @@ static int cls_destroy(void *key, void *datum, void *p)
> e = constraint->expr;
> while (e) {
> ebitmap_destroy(&e->names);
> + if (p->policyvers >= POLICYDB_VERSION_CONSTRAINT_NAMES) {
> + type_set_destroy(e->type_names);
> + kfree(e->type_names);
> + }
Should we have a constraint_expr_destroy() as we do in libsepol?
> etmp = e;
> e = e->next;
> kfree(etmp);
> @@ -643,6 +661,10 @@ static int cls_destroy(void *key, void *datum, void *p)
> e = constraint->expr;
> while (e) {
> ebitmap_destroy(&e->names);
> + if (p->policyvers >= POLICYDB_VERSION_CONSTRAINT_NAMES) {
> + type_set_destroy(e->type_names);
> + kfree(e->type_names);
> + }
So we don't have to repeat this?
> etmp = e;
> e = e->next;
> kfree(etmp);
> @@ -775,7 +797,7 @@ void policydb_destroy(struct policydb *p)
>
> for (i = 0; i < SYM_NUM; i++) {
> cond_resched();
> - hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
> + hashtab_map(p->symtab[i].table, destroy_f[i], p);
Couldn't we avoid the need for passing down the policydb and checking
the version just by ensuring e->type_names gets initialized to NULL and
then the type_set_destroy() and kfree() calls degenerate to no-ops? I
suppose this works, but it complicates the code a bit.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
