Re: [PATCH 1/1] smack: removal of global rule list

[Casey Schaufler]

On 3/7/2019 3:25 AM, Vishal Goel wrote:
> In this patch, global rule list has been removed. Now all
> smack rules will be read using "smack_known_list". This list contains
> all the smack labels and internally each smack label structure
> maintains the list of smack rules corresponding to that smack label.
> So there is no need to maintain extra list.
>
> 1) Small Memory Optimization
> For eg. if there are 20000 rules, then it will save 625KB(20000*32),
> which is critical for small embedded systems.
> 2) Reducing the time taken in writing rules on load/load2 interface
> 3) Since global rule list is just used to read the rules, so there
> will be no performance impact on system
>
> Signed-off-by: Vishal Goel <vishal.goel@xxxxxxxxxxx>
> Signed-off-by: Amit Sahrawat <a.sahrawat@xxxxxxxxxxx>

Looks fine. I will take it for 5.2.

> ---
>   security/smack/smackfs.c | 53 ++++++++++++++----------------------------------
>   1 file changed, 15 insertions(+), 38 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index f6482e5..2a8a1f5 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -67,7 +67,6 @@ enum smk_inos {
>   /*
>    * List locks
>    */
> -static DEFINE_MUTEX(smack_master_list_lock);
>   static DEFINE_MUTEX(smack_cipso_lock);
>   static DEFINE_MUTEX(smack_ambient_lock);
>   static DEFINE_MUTEX(smk_net4addr_lock);
> @@ -134,15 +133,7 @@ enum smk_inos {
>   
>   /*
>    * Rule lists are maintained for each label.
> - * This master list is just for reading /smack/load and /smack/load2.
>    */
> -struct smack_master_list {
> -	struct list_head	list;
> -	struct smack_rule	*smk_rule;
> -};
> -
> -static LIST_HEAD(smack_rule_list);
> -
>   struct smack_parsed_rule {
>   	struct smack_known	*smk_subject;
>   	struct smack_known	*smk_object;
> @@ -211,7 +202,6 @@ static void smk_netlabel_audit_set(struct netlbl_audit *nap)
>    * @srp: the rule to add or replace
>    * @rule_list: the list of rules
>    * @rule_lock: the rule list lock
> - * @global: if non-zero, indicates a global rule
>    *
>    * Looks through the current subject/object/access list for
>    * the subject/object pair and replaces the access that was
> @@ -223,10 +213,9 @@ static void smk_netlabel_audit_set(struct netlbl_audit *nap)
>    */
>   static int smk_set_access(struct smack_parsed_rule *srp,
>   				struct list_head *rule_list,
> -				struct mutex *rule_lock, int global)
> +				struct mutex *rule_lock)
>   {
>   	struct smack_rule *sp;
> -	struct smack_master_list *smlp;
>   	int found = 0;
>   	int rc = 0;
>   
> @@ -258,22 +247,6 @@ static int smk_set_access(struct smack_parsed_rule *srp,
>   		sp->smk_access = srp->smk_access1 & ~srp->smk_access2;
>   
>   		list_add_rcu(&sp->list, rule_list);
> -		/*
> -		 * If this is a global as opposed to self and a new rule
> -		 * it needs to get added for reporting.
> -		 */
> -		if (global) {
> -			mutex_unlock(rule_lock);
> -			smlp = kzalloc(sizeof(*smlp), GFP_KERNEL);
> -			if (smlp != NULL) {
> -				smlp->smk_rule = sp;
> -				mutex_lock(&smack_master_list_lock);
> -				list_add_rcu(&smlp->list, &smack_rule_list);
> -				mutex_unlock(&smack_master_list_lock);
> -			} else
> -				rc = -ENOMEM;
> -			return rc;
> -		}
>   	}
>   
>   out:
> @@ -540,9 +513,9 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>   
>   		if (rule_list == NULL)
>   			rc = smk_set_access(&rule, &rule.smk_subject->smk_rules,
> -				&rule.smk_subject->smk_rules_lock, 1);
> +				&rule.smk_subject->smk_rules_lock);
>   		else
> -			rc = smk_set_access(&rule, rule_list, rule_lock, 0);
> +			rc = smk_set_access(&rule, rule_list, rule_lock);
>   
>   		if (rc)
>   			goto out;
> @@ -636,21 +609,23 @@ static void smk_rule_show(struct seq_file *s, struct smack_rule *srp, int max)
>   
>   static void *load2_seq_start(struct seq_file *s, loff_t *pos)
>   {
> -	return smk_seq_start(s, pos, &smack_rule_list);
> +	return smk_seq_start(s, pos, &smack_known_list);
>   }
>   
>   static void *load2_seq_next(struct seq_file *s, void *v, loff_t *pos)
>   {
> -	return smk_seq_next(s, v, pos, &smack_rule_list);
> +	return smk_seq_next(s, v, pos, &smack_known_list);
>   }
>   
>   static int load_seq_show(struct seq_file *s, void *v)
>   {
>   	struct list_head *list = v;
> -	struct smack_master_list *smlp =
> -		list_entry_rcu(list, struct smack_master_list, list);
> +	struct smack_rule *srp;
> +	struct smack_known *skp =
> +		list_entry_rcu(list, struct smack_known, list);
>   
> -	smk_rule_show(s, smlp->smk_rule, SMK_LABELLEN);
> +	list_for_each_entry_rcu(srp, &skp->smk_rules, list)
> +		smk_rule_show(s, srp, SMK_LABELLEN);
>   
>   	return 0;
>   }
> @@ -2352,10 +2327,12 @@ static ssize_t smk_write_access(struct file *file, const char __user *buf,
>   static int load2_seq_show(struct seq_file *s, void *v)
>   {
>   	struct list_head *list = v;
> -	struct smack_master_list *smlp =
> -		list_entry_rcu(list, struct smack_master_list, list);
> +	struct smack_rule *srp;
> +	struct smack_known *skp =
> +		list_entry_rcu(list, struct smack_known, list);
>   
> -	smk_rule_show(s, smlp->smk_rule, SMK_LONGLABEL);
> +	list_for_each_entry_rcu(srp, &skp->smk_rules, list)
> +		smk_rule_show(s, srp, SMK_LONGLABEL);
>   
>   	return 0;
>   }